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

#2979 Add strict type inference overload for combineReducers. #3484

Merged
merged 4 commits into from
Aug 10, 2019
Merged

#2979 Add strict type inference overload for combineReducers. #3484

merged 4 commits into from
Aug 10, 2019

Conversation

Shakeskeyboarde
Copy link
Contributor

  • Strictly infers state shape and actions from the reducers object map.
  • Updated tests and comments to match new strict type inference.
  • Tested and working in typescript 2.8.

…pe and actions from the reducers object map.
@Shakeskeyboarde Shakeskeyboarde changed the title Add strict type inference overload for combineReducers. #2979 Add strict type inference overload for combineReducers. Jul 28, 2019
@netlify
Copy link

netlify bot commented Jul 28, 2019

Deploy preview for redux-docs ready!

Built with commit e26cb19

https://deploy-preview-3484--redux-docs.netlify.com

@Shakeskeyboarde
Copy link
Contributor Author

This is a fix for issue #2979.

@timdorr
Copy link
Member

timdorr commented Jul 29, 2019

This is basically what #3411 was doing, but without the BC breakage. I think this is good, but if others want to chime in, feel free.

Copy link
Contributor Author

@Shakeskeyboarde Shakeskeyboarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies 😊. Habit from personal projects. Thanks for the heads up and fix.

@timdorr
Copy link
Member

timdorr commented Jul 29, 2019

BTW, we're testing against 3.5.3 now, so compatibility with older versions isn't really of concern. I've found most people upgrade TS pretty regularly.

@pfgray
Copy link

pfgray commented Aug 7, 2019

Should we be encouraging shapes of reducers that include any type other than Action<any> as the second parameter? I'm afraid that reducers with specific action types aren't reflective of reality, since redux sends all actions to all reducers (even "private" redux actions).

For example, suppose we have this seemingly innocuous reducer:

type CounterAction = IncrementAction | DecrementAction
type IncrementAction = {type: "increment", amount: number}
type DecrementAction = {type: "decrement", amount: number}

function counterReducer(state: number = 0, action: CounterAction): number {
  if(action.type === "increment") {
    return state + action.amount;
  } else {
    return state - action.amount;
  }
}

The counterReducer believes it will only ever receive CounterActions, but we know this isn't true, since all redux reducers receive all actions!

This particular reducer produces a state of NaN, since TS indicates it's safe to assume any action that's not IncrementAction is a DecrementAction.

@Shakeskeyboarde
Copy link
Contributor Author

@pfgray The intention here was to make the combined reducer consistent with the typing of the individual reducers.

You're right that maybe individual reducers should use AnyAction instead of specific action types. If you did that, then this would faithfully keep that AnyAction type on the combined reducer.

But, if you do use more specific types for your reducer action parameters, then currently that typing is stripped by combining.

Personally, I do specify types for my actions. But, I do it knowing I can't use an implicit else/default in my reducer. I add the types for testing (so I know when I'm testing an expected action), and for action property types when writing the code that consumes the action.

@pfgray
Copy link

pfgray commented Aug 8, 2019

But, I do it knowing I can't use an implicit else/default in my reducer.

I think we should make that implicit assumption explicit in the type system! That way less astute developers (like myself!) don't get tripped up.

I think the right approach would be to remove the first signature of combineReducers, which would prevent specific action-typed reducers, and would make this change unnecessary.

@timdorr
Copy link
Member

timdorr commented Aug 10, 2019

So, aside from the AnyAction discussion, which is valid but better suited for another issue/PR, I think we're good here. Let's merge this in.

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.

3 participants