-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Proposal: Modifying Typescript typing to make combineReducers match the state object better #2238
Comments
To be honest, none of us three primary maintainers have any real interaction with either of the Typescript or Flow typings. Those definitions have been worked on by others. I particularly know nothing about the nuances of those type systems, so I've avoided the discussions. I know there's been some active issues that have involved changes. If you've got ideas for improving things, probably best to ping some of the people who have worked on them previously, collaborate, and put together changes. If there's agreement that the changes are improvements, we can merge them in. |
Got it :) Do you happen to know some of the relevant contributors off the top of your head? If not, I'll go through some of the commits. |
@aikoven , @timdorr , @raineorshine , @Igorbek , @use-strict - you've helped maintain the TypeScript typings for Redux. Could you comment on the above proposal if you've got a few minutes? I'm hoping this can make Redux a bit more type safe for future users :) |
Already merged into the |
Can't beat the experts, I see :D |
Do you want to request a feature or report a bug?
Feature
What is the current behavior?
Right now, if I have a state defined by
and define my reducers like this:
Typescript will fail to catch two potential errors:
Note the definitions that currently exist regarding combineReducers:
and
What is the proposed behavior?
If we modify the definition of ReducersMapObject to
Then this automatically catches our errors and we get the following two errors:
First, we get
and when we fix that by removing extraProperty, we get
which is everything we've wanted.
Note that this change is a breaking change, since we are converting ReducersMapObject from an interface to just a type. I am not sure whether we can get the same effect with an interface, though if we want to keep the current definition of ReducersMapObject we could just add the type right underneath it like
and then modify combineReducers to be
export function combineReducers<S>(reducers: StateAwareReducersMapObject<S>): Reducer<S>;
This way, people who relied on the old ReducersMapObject can keep using it, but the TypeScript compiler will start showing them errors when their reducers do not add up to create the state.
I haven't thought through all the edge cases (what if the object passed to combineReducers breaks up its responsibilities to delegated method calls that spread their result into the object), but it seems doing something along the lines of the proposal above would go a long way to making combineReducers more type safe. Sorry if I'm discussing a well-known approach or I'm overstepping my bounds here :)
At the very least we can maybe just add the StateAwareReducersMapObject type to the type definition for people to optionally use. Could still provide a lot of benefits. (note: keyof is only available in TypeScript >= 2.1)
What are your thoughts?
Which versions of Redux, and which browser and OS are affected by this issue? Did this work in previous versions of Redux?
Latest Redux
The text was updated successfully, but these errors were encountered: