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

Inferred dispatch type is not helpful #2896

Closed
eduter opened this issue Nov 11, 2022 · 21 comments · Fixed by #2964
Closed

Inferred dispatch type is not helpful #2896

eduter opened this issue Nov 11, 2022 · 21 comments · Fixed by #2964
Milestone

Comments

@eduter
Copy link

eduter commented Nov 11, 2022

I was hoping that the AppDispatch from the examples would only accept the actions I defined, but it accepts basically anything.

Was that a design decision, a bug, a known limitation from the current type definition, a limitation from TypeScript itself?

I've created this repo to illustrate what I mean:
https://github.com/eduter/redux-toolkit-example

More specifically these lines:
https://github.com/eduter/redux-toolkit-example/blob/39b4e0d/src/features/counter/Counter.tsx#L48
https://github.com/eduter/redux-toolkit-example/blob/39b4e0d/src/features/counter/Counter.tsx#L26
https://github.com/eduter/redux-toolkit-example/blob/39b4e0d/src/features/counter/Counter.tsx#L34

@markerikson
Copy link
Collaborator

This is intentional.

We do not find it useful to try to limit what types of actions can be dispatched at the TS level. Any reducer can respond to any action, and any action can be dispatched without a single reducer caring about it - that's an intentional part of Redux's design. Because of that, we specifically recommend against the semi-common practice of trying to create TS unions representing action types:

https://redux.js.org/usage/usage-with-typescript#avoid-action-type-unions

The main goal of inferring an AppDispatch type is to ensure that TS recognizes any modifications to dispatch that come from middleware overriding behavior (most commonly, the redux-thunk middleware allowing dispatching thunk functions).

Is there a specific reason you're trying to limit things at the TS level?

@phryneas
Copy link
Member

Adding to that: every middleware you have in your application can send any kind of action to your reducers - and many of them also will. There is no real value in limiting the action type at dispatch. Just use an action creator and don't dispatch hand-written actions and you should be pretty much as safe as it makes sense.

@eduter
Copy link
Author

eduter commented Nov 11, 2022

Right, if you think of AppDispatch as the type that has to accept any action that might be dispatched by anyone in the application, that makes sense. What I was looking for was a type for actions my components may dispatch, to avoid mistakes when connecting them. They won't dispatch INIT or any middleware actions. But sticking to action creators avoids most problems. I think the only problem it doesn't solve, probably can't even be caught at compilation time:

https://github.com/eduter/redux-toolkit-example/blob/39b4e0d/src/features/counter/Counter.tsx#L34

In past projects I didn't have this problem because I was writing all the boilerplate by hand. When writing my own action creators, they would only use the parameters they expected, while the ones generated by the toolkit will put anything they get (including mouse events) into the payload property. I guess it's a fair price to pay for all the code it saves me from writing.

@phryneas
Copy link
Member

Well, let's go through these errors:

dispatch(() => 'not an action')}

in this case, () => 'not an action' is a valid thunk. Even if you have a dispatch type that only accepts certain plain actions, if you use the redux-thunk middleware, this has to be a valid thing to dispatch.

          onClick={increment} // A non-serializable value was detected in an action, in the path: `payload`.

This one is... TypeScript being TypeScript - and overloads. The method signature of the action creator here is () => SomeAction, but the code actually assumes that this will never be called with any argument.
Unfortunately, TS allows a function with the signature () => SomeAction to be used in place of a method with the signature (someArgument) => SomeAction - so here, event is put into the payload.

I wonder if we could catch that, but it would not be in the type of dispatch. It would be a change in the action creator type.

@phryneas
Copy link
Member

phryneas commented Nov 11, 2022

@markerikson I think we could catch that second case with the change

export interface ActionCreatorWithoutPayload<T extends string = string>
  extends BaseActionCreator<undefined, T> {
  /**
   * Calling this {@link redux#ActionCreator} will
   * return a {@link PayloadAction} of type `T` with a payload of `undefined`
   */
-  (): PayloadAction<undefined, T>
+  (noArgument: void): PayloadAction<undefined, T>
}

@markerikson markerikson added this to the 1.9.x milestone Nov 12, 2022
@markerikson
Copy link
Collaborator

sure, seems reasonable to get into 1.9.1!

@markerikson
Copy link
Collaborator

markerikson commented Nov 30, 2022

That @phryneas is a computer genius man! (classical reference)

image

@markerikson
Copy link
Collaborator

@maksnester
Copy link

Hey guys, I'm trying to understand the change that was done here with that noArgument: void as it messes with components when they declare fn prop type via MouseEventHandler.

I see that exact error @markerikson attached as a screenshot but I personally don't understand why it is something good or helpful. What's the problem with the mouse event being passed into payload here if the action doesn't use it (so it'll be ignored) ?

In many cases I just want a component to call whatever I pass to it like

<Dialog onClose={reduxCloseHandlerWithDispatchAttached} />

And because of that change with noArgument: void I have to make dummy wrappers like

<Dialog onClose={() => reduxCloseHandlerWithDispatchAttached()} />

Just to prevent that mouse event being passed to the action that doesn't even care about it. From reading this thread I understood it is a trade off to avoid

// A non-serializable value was detected in an action, in the path: payload

but is non-serializable value even a problem if it doesn't end up in the state?

@EskiMojo14
Copy link
Collaborator

@maksnester actions also need to be serializable, to allow proper time travel debugging. actions are recorded to allow the devtools to support skipping and reordering them.

@adrian-gierakowski
Copy link

I’m only beginning to learn about redux and RTK and was also surprised that I was able to dispatch actions which are not supported by the store.

Is there any resource I could learn about why being able to do that in my application code was desirable? Thanks!

@markerikson
Copy link
Collaborator

@adrian-gierakowski : conceptually, you can think of dispatching actions like an event emitter. You could do emitter.trigger("some-irrelevant-event"), and there might be 0, 1, or many listeners registered to handle that event. In the same way, when you dispatch an action, there could be 0, 1, or many slice reducers that update state in response to that action, and a reducer also wouldn't know ahead of time about all the possible actions you might dispatch.

@adrian-gierakowski
Copy link

@adrian-gierakowski : conceptually, you can think of dispatching actions like an event emitter. You could do emitter.trigger("some-irrelevant-event"), and there might be 0, 1, or many listeners registered to handle that event. In the same way, when you dispatch an action, there could be 0, 1, or many slice reducers that update state in response to that action, and a reducer also wouldn't know ahead of time about all the possible actions you might dispatch.

Thank you for your reply @markerikson!

I still have trouble understanding why one would want to dispatch an action for which there is no registered handler in the store. Could you give me a real life example of such situation? Do stores\reducers tend to be modified on the fly with handlers registered and de-registered at runtime. Even in such situation, I think we should be able to know, at build (type checking) time, the set of all possible handlers which might become registered at some point during lifetime of the application.

@EskiMojo14
Copy link
Collaborator

@adrian-gierakowski it's important to remember that you aren't the only one dispatching to your store. Any middleware you add has the ability to dispatch actions, and Redux itself has internal actions it will dispatch - INIT being the obvious one, which is actually randomised to prevent reducers from handling it as a special case.

@phryneas
Copy link
Member

@adrian-gierakowski there will always be at least one action in dev that cannot be part of your types, and that's the Redux store's INIT action, which in dev has a randomized action type.
Middleware will probably add a lot of additional actions on top of that.

As a result, you can never rely on the completeness of a hand-written action type - and at the same time, even maintaining the "almost complete" type by hand is a lot of work.

Since it's not complete, you cannot use it in reducers, as you will end up with reducer functions that have a "too tight" type signature.

And on the side of dispatch, you either put in a lot of work into that "almost complete" type, or you make the pragmatic choice to just never dispatch hand-written actions, but instead always call an action creator.

Since you get action creators for free, the decision between "almost no work" and "a lot of work" should be easy in almost all cases.

@adrian-gierakowski
Copy link

thank you both

and Redux itself has internal actions it will dispatch - INIT being the obvious one

Ok, but I guess users of redux wouldn't normally dispatch these from their application code? Such special events could be dispatched via an additional, untyped dispatch method

Middleware will probably add a lot of additional actions on top of that.

If middlewares are added statically, I don't see why they shouldn't be able to register, at type level, actions which they can handle. If a more dynamic behaviour is required, again, a dispatchUnsafe method could facilitate it

never dispatch hand-written actions, but instead always call an action creator.
Since you get action creators for free, the decision between "almost no work" and "a lot of work" should be easy in almost all cases.

As far as I can from a sample app created with vite-template-redux, existence of creators in the code base does not guarantee that they have been registered in the store, so the only guarantee they provide is that the payload matches the even name (type).

Seems to me that in 98% of cases, dispatching an action which has not been registered in the store would be a programming error, and one which could be avoided by better types. I'm not claiming that it would be easy to come up with such types, just that it would be desirable.

@phryneas
Copy link
Member

How would that change if you add that action to your store's types - how would you ever find out that none of your reducers actually reacts to it?

If you want that guaranteed:

  • use slices with combineSlices with lazy reducer injection. The moment you import an action, the slice that created that action will be injected. You will technically not be able to dispatch an action without a reducer for it in place.

@adrian-gierakowski
Copy link

If you want that guaranteed:

use slices with combineSlices with lazy reducer injection. The moment you import an action, the slice that created that action will be injected. You will technically not be able to dispatch an action without a reducer for it in place.

Thanks! Would you be able to point me in direction of some resources, docs, or ideally sample\template project from which I could learn more about this approach?

How would that change if you add that action to your store's types - how would you ever find out that none of your reducers actually reacts to it?

I'm not sure I understand the question, but it's probably due lack of fundamental understanding on my side. In general I'd expect an api for creating composing slices and creating a store to be able to automatically produce a type which accurately reflects the capabilities of the store it produces

@EskiMojo14
Copy link
Collaborator

Code splitting docs
combineSlices API reference
PR adding an example of lazy injection

@EskiMojo14
Copy link
Collaborator

For what it's worth, you could write up a quick middleware to log when state doesn't change as a result of an action being dispatched:

const noopMiddleware: Middleware = ({ getState }) => (next) => (action) => {
  const stateBefore = getState();
  const result = next(action);
  const stateAfter = getState();
  if (stateBefore === stateAfter) console.log("State did not change when action was dispatched", action);
  return result;
}

But honestly this seems like a non-issue to me, part of Redux's principles is that reducers should be able to handle any action, and returning the same state is an entirely valid response.

@markerikson
Copy link
Collaborator

Yeah - overall it feels like you're looking for a solution to a problem that doesn't exist or is irrelevant :)

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.

6 participants