Skip to content

createSlice: correctly infer state in TS 4.8 #2547

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

Closed
wants to merge 6 commits into from

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Jul 26, 2022

fixes #2543

@netlify
Copy link

netlify bot commented Jul 26, 2022

Deploy Preview for redux-starter-kit-docs ready!

Name Link
🔨 Latest commit 62b771b
🔍 Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/62eeaf83a38c41000888215c
😎 Deploy Preview https://deploy-preview-2547--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 26, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 62b771b:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
rsk-github-issues-example Configuration
@examples-query-react/basic Configuration
@examples-query-react/advanced Configuration
@examples-action-listener/counter Configuration

@phryneas phryneas force-pushed the pr/createSlice-infer-state branch 3 times, most recently from c442388 to 7219d19 Compare July 26, 2022 19:27
@phryneas
Copy link
Member Author

This PR contains the fix - but right now I've commented the fix out and am still not getting the error in CI... I'll have to drop this. @Shrugsy do you maybe have resources to play around and find out why the CI isn't doing here?

@Shrugsy
Copy link
Collaborator

Shrugsy commented Jul 26, 2022

This PR contains the fix - but right now I've commented the fix out and am still not getting the error in CI... I'll have to drop this. @Shrugsy do you maybe have resources to play around and find out why the CI isn't doing here?

I have a suspicion that the GH actions yaml file may not be formatted correctly, so it may run the "tsc --version" line, but not actually run the type tests. I won't have a chance to look at it properly for a couple of days or so. It might be worth removing the "tsc --version" line temporarily to see whether this is the case.

@Shrugsy
Copy link
Collaborator

Shrugsy commented Jul 27, 2022

This PR contains the fix - but right now I've commented the fix out and am still not getting the error in CI... I'll have to drop this. @Shrugsy do you maybe have resources to play around and find out why the CI isn't doing here?

I have a suspicion that the GH actions yaml file may not be formatted correctly, so it may run the "tsc --version" line, but not actually run the type tests. I won't have a chance to look at it properly for a couple of days or so. It might be worth removing the "tsc --version" line temporarily to see whether this is the case.

☝️ Looks like a dead end - not the issue

@Meligy
Copy link

Meligy commented Aug 3, 2022

Hello,
I know you might want to wait until you fix the type tests runner etc., but thought it might be useful that I tried this in a real customer project just now, one that started failing when I tried the next version of TypeScript on it (and it resolved to 4.8.0-dev.20220803). And I can confirm that the change to ValidateSliceCaseReducers in this PR fixed the problem. 👍

I tested it by modifying node_modules/@reduxjs/toolkit/dist/createSlice.d.ts directly, as I just wanted to test it and report here, and patching it in a custom declaration file didn't seem to work, probably because it also requires copying createSlice and CreateSliceOptions in the custom declaration file.

Which I also later did to see if I can use that as a temporary workaround. I added the following to the react-app-env.d.ts file that came with the create-react-app template:

// Workaround for `createSlice` reducer functions
//   not inferring the state type automatically in TypeScript 4.8
// Should be removed when this PR is released:
//   https://github.com/reduxjs/redux-toolkit/pull/2547
module "@reduxjs/toolkit" {
  export * from "@reduxjs/toolkit/dist";
  import type {
    CreateSliceOptions as original_CreateSliceOptions,
    Slice,
    SliceCaseReducers,
  } from "@reduxjs/toolkit/dist";

  // The main change in the PR
  export type ValidateSliceCaseReducers<
    S,
    ACR extends SliceCaseReducers<S>
  > = ACR & {
    [T in keyof ACR]: ACR[T] extends {
      reducer(s: S, action?: infer A): any;
    }
      ? {
          prepare(...a: never[]): Omit<A, "type">;
        }
      : ACR[T];
  };

  // The following types are copied from `@reduxjs/toolkit` with minimal changes
  //    as if we do not override them, `createSlice` will not use our custom `ValidateSliceCaseReducers`
  // So we override `createSlice`'s `CreateSliceOptions`
  //    and change the `reducers` option to point to our custom `ValidateSliceCaseReducers`

  export interface CreateSliceOptions<
    State = any,
    CR extends SliceCaseReducers<State> = SliceCaseReducers<State>,
    Name extends string = string
  > extends original_CreateSliceOptions<State, CR, Name> {
    reducers: ValidateSliceCaseReducers<State, CR>;
  }

  export declare function createSlice<
    State,
    CaseReducers extends SliceCaseReducers<State>,
    Name extends string = string
  >(
    options: CreateSliceOptions<State, CaseReducers, Name>
  ): Slice<State, CaseReducers, Name>;
}

Thanks a lot for providing the solution, and I hope it can be released soon.

@phryneas
Copy link
Member Author

phryneas commented Aug 5, 2022

@Meligy you can also always try the latest CodeSandbox build for a PR - in this case you find install instructions here: https://ci.codesandbox.io/status/reduxjs/redux-toolkit/pr/2547/builds

@phryneas
Copy link
Member Author

phryneas commented Aug 5, 2022

So with this change (which unfortunately adds a dependency), the patch will only be applied to TS versions >= 4.8, as it would break older TS versions.

Unfortunately we can't really test that at the moment as the current "next" release still labels itself as 4.7.

@phryneas
Copy link
Member Author

phryneas commented Aug 5, 2022

Now it works already with pre-release versions.

The type bugs that are still appearing are legit 4.8 TS bugs, but only occur in the "slice wrapped in generics" case, so with a whole level of complexity added on top.

I'd suggest we get this fix out as a 1.8.* regardless of that as it will already alleviate 95% of errors people will have when upgrading to TS 4.8.

As I'll be going on my vacation tomorrow I don't think I'll be able to look deep enough into the "generics error case".

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 this pull request may close these issues.

State argument on createSlice is no longer inferred with typescript beta 4.8 and they do not plan to fix it
4 participants