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

Action payload is still possibly undefined even when a value is given #399

Closed
maxijonson opened this issue Feb 28, 2020 · 6 comments
Closed

Comments

@maxijonson
Copy link

Considering this example:

type TestType = { counter: number; step: number; };
const test = createSlice({
    name: "test",
    initialState: { counter: 0, step: 1 } as TestType,
    reducers: {
        increment: (state) => {
            state.counter += state.step;
        },
        setCounterAndStep: (state, action: PayloadAction<TestType>) => {
            state.counter = action.payload.counter;
            state.step = action.payload.step;
        }
    }
});

When calling the setCounterAndStep action with an empty object as payload... (or an incomplete object)

test.actions.setCounterAndStep({  })

the function tries to apply the payload type to undefined AND TestType, which comes from two function overloads when it really should be applying it only to TestType, since an empty object is not undefined.

No overload matches this call.
  Overload 1 of 2, '(payload?: undefined): { payload: undefined; type: string; }', gave the following error.
    Argument of type '{}' is not assignable to parameter of type 'undefined'.
  Overload 2 of 2, '(payload?: { counter: number; step: number; }): { payload: { counter: number; step: number; }; type: string; }', gave the following error.
    Argument of type '{}' is not assignable to parameter of type '{ counter: number; step: number; }'.
      Type '{}' is missing the following properties from type '{ counter: number; step: number; }': counter, stepts(2769)

Additionally, Intellisense fails to suggest counter and step because of the undefined overload.

When trying to understand why this happens, I stumbled upon the ActionCreatorWithOptionalPayload interface of typings.d.ts. It seems the first overload is described by

(payload?: undefined): PayloadAction<undefined, T>;

and the second by

<PT extends Diff<P, undefined>>(payload?: PT): PayloadAction<PT, T>;

I found the first overload strange... Why would the payload be optional AND typed as undefined? I think there should only be the second overload, which already has payload optional, so it has the same effect as the first overload.

I removed the first overload from the typings and it fixed the issue. All of these calls work as expected:

test.actions.setCounterAndStep({}) // Error as expected. Type '{}' is missing the following properties from type '{ counter: number; step: number; }': counter, step
test.actions.setCounterAndStep({ counter: 0, step: 2 }) // Good. Type is satisfied
test.actions.increment() // Good. Payload is undefined

Additionally, Intellisense now suggests correctly counter and step for setCounterAndStep.

Is there something I am not seeing? Why do we need the first overload in the first place when we can just leave payload optional on the second overload and have the same effect?

Using TS 3.7.5, also tried with TS 3.8.2

@markerikson
Copy link
Collaborator

Paging @phryneas ...

@phryneas
Copy link
Member

phryneas commented Feb 28, 2020

I'm going to take a guess and say that you have strictNullChecks set to false, which means that TypeScript internally handles your code as

        setCounterAndStep: (state, action: PayloadAction<TestType | null | undefined>  | null | undefined) => {

So our types are handling your code as that.

Let's step back and take a look at what should happen in that case:

In the case that a user types their PayloadAction as X | undefined we have to make sure that it actually possible for that user to call their action creator three ways:

  1. without an argument
  2. with undefined as argument
  3. with X as argument

The overload that is puzzling you is there to allow for 2. in these cases.

I've written that half a year ago, so I'm not 100% sure any more, but I guess it couldn't be defined as just the second overload, as the undefined would have widened the extends clause to accept just any type - so I had to remove undefined from that type and introduce an extra overload for that case.

Anyways: I would really suggest you turn on strictNullChecks in your code base. You are losing a lot of type safety, and TypeScript really isn't meant to be used without that feature - that's just there for pre-Typescript-2-backwards-compatiblity and in-migration settings.

@maxijonson
Copy link
Author

That was it! Enabling strictNullChecks fixes the issue. I will still have to find a temporary workaround until I keep it enabled since I cannot enable it at the moment for other reasons unrelated to redux-toolkit... Thank you for your fast replies!

@phryneas
Copy link
Member

If you find a way of handling that that works well with both strictNullChecks true AND false, a PR is very welcome :)

@phryneas
Copy link
Member

Hey @maxijonson, I put in a PR that might improve the autocompletion for people with strictNullChecks: false (#428) - could you give it a test and give me feedback if it improves the situation to before?

yarn add https://pkg.csb.dev/reduxjs/redux-toolkit/commit/982da2ca/@reduxjs/toolkit
npm i https://pkg.csb.dev/reduxjs/redux-toolkit/commit/982da2ca/@reduxjs/toolkit

@maxijonson
Copy link
Author

Just tested it and it works! I get autocompletion with the correct types and actions that do not have a payload will fail if I try to pass one in (as expected). Thanks you very much! 🙂

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

No branches or pull requests

3 participants