Skip to content

Fix combineReducers typings. #3679

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

Merged
merged 3 commits into from
Jul 14, 2020

Conversation

gilbsgilbs
Copy link
Contributor

@gilbsgilbs gilbsgilbs commented Jan 14, 2020

PR Type

Does this PR add a new feature, or fix a bug?

Fixes a bug.

Why should this PR be included?

It fixes a typing issue I and some other folks have with the combineReducers function: #2709

Checklist

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Is there an existing issue for this PR?
  • Have the files been linted and formatted?
  • Have the docs been updated to match the changes in the PR?
  • Have the tests been updated to match the changes in the PR?
  • Have you run the tests locally to confirm they pass?

Bug Fixes

What is the current behavior, and the steps to reproduce the issue?

Unfortunately, I failed to create a simple reproduction example. Still, it happens that TypeScript would fail to infer the proper generic type to combineReducers function. In my case, I'm doing something like this:

interface FooState {}
interface AppAction extends Redux.Action<string> {}
interface FooAction0 extends AppAction {}
interface FooAction1 extends AppAction {}
type FooActions = FooAction0 | FooAction1;

function fooReducer(state: FooState, action: FooActions): FooState {
  return state;
}

// Type error:
// Argument of type 'Reducer<CombinedState<{ fooReducer: FooState; }>, FooActions>' is not assignable to parameter of type 'Reducer<CombinedState<{ fooReducer: FooState; }>, AnyAction>'.
combineReducers({ fooReducer: fooReducer });

// works
combineReducers<{fooReducer: FooState}>({ fooReducer: fooReducer });

What is the expected behavior?

I would expect TypeScript to infer a generic type automatically.

How does this PR fix the problem?

It removes explicit anys which appears to help TypeScript to infer a type. I'm not sure why though 🤷‍♂️ . Could this be a bug in TypeScript?

More specifically, the first generic type of ReducersMapObject is a state which is already any by default, and the second generic type is A extends Action which is Action by default and not any. So the fact that the second generic type is any confuses TS for some reason.

@netlify
Copy link

netlify bot commented Jan 14, 2020

Deploy preview for redux-docs ready!

Built with commit a54a229

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

@timdorr
Copy link
Member

timdorr commented Jan 14, 2020

Should ReducersMapObject's second arg default be AnyAction instead?

@gilbsgilbs
Copy link
Contributor Author

Should ReducersMapObject's second arg default be AnyAction instead?

It sounds reasonable to me. I have checked against my local project though and it doesn't improve the type inference in this case.

I also see another usage of ReducersMapObject<any, any> in StateFromReducersMapObject. Should I also update this one to ReducerMapObject without specifiying generics?

@gilbsgilbs
Copy link
Contributor Author

@timdorr I don't want to be "that guy" you know, but is there anything I can do to help getting this merged and released anytime soon? 😉

@jonjaques
Copy link

jonjaques commented Jan 21, 2020

Can confirm that this fixes a regression we experienced recently. Specifically, our thunk dispatch type was conflicting w/ a third party reducer (TS complaining about actions missing type & payload.

Thanks @gilbsgilbs!

Erm, edit: Unfortunately this is still conflicting w/ one less property "type"

@timdorr
Copy link
Member

timdorr commented Feb 10, 2020

Only thing this needs is some tests!

@timdorr
Copy link
Member

timdorr commented Jul 14, 2020

I guess I'll (finally) merge this in without tests. Thanks!

@timdorr timdorr merged commit 78716bf into reduxjs:master Jul 14, 2020
alokreddy pushed a commit to alokreddy/redux that referenced this pull request Sep 30, 2020
@reduxjs reduxjs deleted a comment from Charles-Chiakwa Nov 20, 2020
webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
Former-commit-id: 05f243c
Former-commit-id: eb4bc7f
webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
Former-commit-id: 05f243c
Former-commit-id: eb4bc7f
webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
Former-commit-id: 05f243c
Former-commit-id: eb4bc7f
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