Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate createAsyncThunk type failures with TS 5.1+ #3758

Closed
markerikson opened this issue Oct 1, 2023 · 9 comments · Fixed by #3777
Closed

Investigate createAsyncThunk type failures with TS 5.1+ #3758

markerikson opened this issue Oct 1, 2023 · 9 comments · Fixed by #3777
Milestone

Comments

@markerikson
Copy link
Collaborator

Bumped the TS versions to include 5.1 and 5.2, and got this:

https://github.com/reduxjs/redux-toolkit/actions/runs/6367491100/job/17286119923?pr=3757

Run yarn tsc --version
Version 5.1.6
Error: src/tests/createAsyncThunk.typetest.ts(294,39): error TS2345: Argument of type 'AsyncThunk<number, number, AsyncThunkConfig>' is not assignable to parameter of type '(arg?: number | undefined) => any'.
  Types of parameters 'arg' and 'arg' are incompatible.
    Type 'number | undefined' is not assignable to type 'number'.
      Type 'undefined' is not assignable to type 'number'.
Error: src/tests/createAsyncThunk.typetest.ts(295,5): error TS2554: Expected 1 arguments, but got 0.
Error: Process completed with exit code 2.
@markerikson markerikson added this to the 2.0 milestone Oct 1, 2023
@EskiMojo14
Copy link
Collaborator

EskiMojo14 commented Oct 1, 2023

not sure why, but this seems to be a Typescript bug. the second API argument makes the difference:
image
image

playground

here's a slightly more abstracted example with the same issue:
image

@Andarist
Copy link

Andarist commented Oct 1, 2023

I bisected the change to this diff so it's almost guaranteed that the change is caused by microsoft/TypeScript#52609

@markerikson
Copy link
Collaborator Author

Oh lovely :)

Don't think there's a good way for us to conditionally // @ts-expect-error for specific TS versions. Not sure what the best workaround is for the typetest here.

@Andarist
Copy link

Andarist commented Oct 1, 2023

Ye, it might be tricky for u to handle this specific diff in ur test case. The bigger q is if this is a desired change or not? I think that likely it is intended on the TS side of things but i didnt actually analyze ur types to be sure

@markerikson
Copy link
Collaborator Author

I don't fully have the behavior here in my head, but it seems like the typetest worked fine as of TS 5.0, and broke with TS 5.1. So, that makes me assume that this is not a desired change.

@Andarist
Copy link

Andarist commented Oct 1, 2023

The PR in question started to consider some inference sources that were previously ignored. So it is expected that some scenarios could get affected. I’m unsure yet if this particular change is for good or not - i only looked at a playground that manifested the change but there was no error there. The inferred type was just different. Could u create a playground that would show the unintended behavior in a more direct way?

@EskiMojo14
Copy link
Collaborator

here's a playground for what createAsyncThunk tries to do. Basically, when the payload creator's argument is optional (or possibly undefined) we want to mark the argument as optional in the resulting function. The new behaviour breaks our ability to infer that, because an optional parameter no longer infers as possibly undefined.

@Andarist
Copy link

Andarist commented Oct 1, 2023

Oh, ye - this looks wrong. Luckily... I already fixed this in microsoft/TypeScript#55397 . You can see that the nightly TS version doesn't suffer from this: TS playground

@markerikson
Copy link
Collaborator Author

Well, #3761 is my first use of https://github.com/phryneas/ts-version :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants