-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add inferred action creator types #975
Conversation
Ooooooo. This looks fascinating. I know one of the weaknesses in our TS support since the beginning has been that the type of the action constants is only a @phryneas , @msutkowski , thoughts? Per the TS 4.1 support: the plan for the upcoming RTK 1.6 release is to drop support for TS < 3.8. However, we can definitely have TS typedefs for < 4.1, and 4.1+. |
O extends GetDefaultMiddlewareOptions = {}, | ||
A extends AnyAction = AnyAction | ||
> = O extends { | ||
thunk: false | ||
} | ||
? never | ||
: O extends { thunk: { extraArgument: infer E } } | ||
? ThunkMiddleware<S, AnyAction, E> | ||
? ThunkMiddleware<S, A, E> | ||
: | ||
| ThunkMiddleware<S, AnyAction, null> //The ThunkMiddleware with a `null` ExtraArgument is here to provide backwards-compatibility. | ||
| ThunkMiddleware<S, AnyAction> | ||
| ThunkMiddleware<S, A, null> //The ThunkMiddleware with a `null` ExtraArgument is here to provide backwards-compatibility. | ||
| ThunkMiddleware<S, A> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because the reducer has an intersection type with ThunkMiddlewareFor, so without it, the reducer will merge the action argument to AnyAction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ThunkMiddlewareFor is passed to configureStore and is merged into reducer here:
export function configureStore<
S = any,
A extends Action = AnyAction,
M extends Middlewares<S> = [ThunkMiddlewareFor<S, {}, A>]
>(options: ConfigureStoreOptions<S, A, M>): EnhancedStore<S, A, M> {
export interface EnhancedStore<
S = any,
A extends Action = AnyAction,
M extends Middlewares<S> = Middlewares<S>
> extends Store<S, A> {
/**
* The `dispatch` method of your store, enhanced by all its middlewares.
*
* @inheritdoc
*/
dispatch: DispatchForMiddlewares<M> & Dispatch<A>
}
Seems like a good idea to me if it can be done - I'm not sure how this can possibly work without restricting |
I've feared this would come up eventually. 🤣 There are some things in here we can do (but I'm not sure if we should do them) and some thing we really shouldn't. Let's get the "shouldn't" part out of the way:
And having all these out of the way, there isn't too much left to actually use these types for. I can think of three usages:
I've actually written a blog post on this whole debacle: https://phryneas.de/redux-typescript-no-discriminating-union There also has been a lot of discussion on this topic over in the TS repo since Redux has been translated to TS: reduxjs/redux#3580 And @markerikson has recently changed the "Usage with TypeScript" section of Redux to remove mentions of the "Action Union Type" pattern: reduxjs/redux#4042 All that said: of course we could - without creating any action union types or adding tools for people to create those (they can still do it by hand if they really want to) - add this to the action creators created by We would need an extra typing build for TS>=4.1 but we'll have to introduce that for So my suggestion would be to create a new type ScopedActionType<Scope extends string, Type extends string> = `${Scope}/${Type}` in a TS<4.1 build, we could just exchange that transparently with type ScopedActionType<Scope extends string, Type extends string> = string and use that in the |
@phryneas good points. What do you think about the relay approach of generating enums? It generates a |
@gtkatakura that will still give you a reducer that has an outer interface that can only be called with certain actions, but will in reality be called by any number of other actions. (See the screenshot in your initial post - that reducer has to be callable with There is any number of ways to handle that, for example reduxjs/redux#4025 (comment) suggested a type like type OtherActions<T extends AnyAction> = { type: T extends T['type'] ? T : { type: unknown } } But all that leaves one thing untouched: That just isn't necessary with RTK. Every RTK action has a if (firstAction.match(action)) {
// ...
} else if (secondAction.match(action)) {
// ...
} without the need of declaring any Action Union types. Going further, that pattern can be used to use matchers that do not only use at the const isActionIAmLookingFor = isRejectedWithValue(asyncThunk1, asyncThunk2)
if (isActionIAmLookingFor(action)) {
} This plays nicely with So in the end, we have something that covers more use cases with less work. I really do not want to encourage people using the old Union Action Type pattern instead of this. With the suggested changes above anyone can still create their own boilerplate by creating a helper type like type ActionsFromSlice<T> = T extends { actions: infer Actions } ? { [K in keyof Actions]: Actions[K] extends (...args: any[]) => infer R ? R : never }[keyof Actions] : never; so it would still be possible - but I would not want to ship actual code to encourage that. |
@phryneas I see all the arguments and I definitely agree with them, there should be no union action types needed and definitely no reducer scoping to just selected actions, however I think there is one opportunity still to consider on this proposal - it would be beneficial to have properly typed action types on actions on their own, without scoping reducers to work only with them. For example, for slice defined like this const slice = createSlice({
name: 'counter',
initialState: 0,
reducers: {
increment: (state: number, action: PayloadAction<number>) => state + action.payload,
decrement: (state: number, action: PayloadAction<number>) => state - action.payload
},
}); it would be nice to be able to have action types defined like this: // { type: 'counter/increment', payload: number }
type IncrementAction = typeof slice.actions.increment;
// { type: 'counter/decrement', payload: number }
type DecrementAction = typeof slice.actions.decrement;
// Still not scoped
// Reducer<number, AnyAction>
type SliceReducer = typeof slice.reducer; What do you think? |
@pbambusek We will consider this for the next release, but for this release, we just have too much stuff already happening with RTK-Query - and this is actually more work than it seems since we would have to adjust the build again to support both TS>4.1 and below. Even though, honestly, I still do not see any reason to ever reference one of those types in code. Hovering them in the IDE for more information, sure. But what do you actually want to do with that? |
Closing in favor of #2250 that adds the literal types for actions, but does not do any more than that. |
Hey, guys. I want to share some ideas about safety infer the action types. This is just a draft of the idea because I am using the template literal types of TS 4.1 and I know have such a version dependency would be problematic for people using old versions of TS. I didn't work into caseReducers and other things, because like I said, I just want to share the idea and see if this will make sense to be part of redux-toolkit in the future or someone know some way to use template literal types and keep support for old versions of TS.
The idea is to infer the
slice.actions
andslice.reducer
to make totally safety the usage of thetype
property of the action.