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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed the definition of 'Reducer<S>' for TypeScript 2.4. #2467

Merged
merged 1 commit into from Jun 28, 2017

Conversation

DanielRosenwasser
Copy link
Contributor

This is going to be a bit roundabout, but bear with me. 馃槃

Stricter checks on generics in TypeScript 2.4 correctly errored on the fact that reducers passed to combineReducers were expected to be more general than they actually needed to be.

Since the A type parameter was never used as part of the return type - it was only used as a constraint - the appropriate next step was to replace the parameter type of A with the Action type directly:

export type Reducer<S> = (state: S, action: Action) => S;

However, because of excess property checks, this introduced errors when object literals were passed directly to a reducer. For example:

const rootReducer: Reducer<RootState> = combineReducers({
  todos: todosReducer,
  counter: counterReducer,
})

const rootState: RootState = rootReducer(undefined, {
  type: 'ADD_TODO',
  text: 'test',
//~~~~~~~~~~~~ excess property
})

Thus, I created a new AnyAction which is just Action with an index signature. This type is separate from Action to avoid polluting subtypes of Action with an index signature.

@jimsugg
Copy link

jimsugg commented Jun 28, 2017

+1 on this. Redux is unusable with Typescript 2.4 without this (or something like it.) See https://github.com/Microsoft/TypeScript/issues/16795

@morlay
Copy link

morlay commented Jun 28, 2017

Could we use another way to fix this.
With the Generics Default (typescript@2.3+)

export type Reducer<S, A extends Action = Action> = (state: S, action: A) => S;
// Dispatch need to update too.
export interface Dispatch<A extends Action = Action> {
    (action: A): A;
}

@timdorr
Copy link
Member

timdorr commented Jun 28, 2017

Thanks for looking into this. Can someone also make a PR against the next branch so things are in sync for the next major release? That's using TS 2.1 (it can be upgraded to 2.4, if you want).

@timdorr timdorr merged commit 0de7e5b into reduxjs:master Jun 28, 2017
@jimsugg
Copy link

jimsugg commented Jun 28, 2017

I tested morlay's suggestion inside my project. The alternative suggestion for the Reducer definition seems to work fine:

export type Reducer<S, A extends Action = Action> = (state: S, action: A) => S;

However, the suggestion for the Dispatch definition causes problems in the form of other compile errors. These may be resolvable, but I need to spend a little more time to verify that. In my project, I have no problem with the original Dispatch definition, and I am not sure if this redefinition is really needed.

@jimsugg
Copy link

jimsugg commented Jun 28, 2017

If you did change the Dispatch definition, it would probably need to be this:

export interface Dispatch<S, A extends Action = Action> {
    (action: A): A;
}

The 'S' parameter was missing in morley's post. I'm still having trouble with even that substitution, however.

@jimsugg
Copy link

jimsugg commented Jun 28, 2017

Thanks for merging Daniel's fix, Tim. I think that's the right way to go for now. Hopefully this will get published soon.

@DanielRosenwasser
Copy link
Contributor Author

Thanks!

@DanielRosenwasser DanielRosenwasser deleted the fixReducer branch June 29, 2017 00:19
@morlay
Copy link

morlay commented Jun 29, 2017

@jimsugg 'S' parameter in Dispatch is not necessary. we can keep S for now, but i think we can drop it in future.

@jimsugg
Copy link

jimsugg commented Jun 29, 2017

@morlay - Yes - that seems right. Not being closely familiar with all the ramifications of the declarations, as soon as I saw there were multiple other references to Dispatch<S> in the file, I just backed away without examining it closely. I did try your proposal with the S in place to see how it would work in my project, and there were several compile errors, but they looked like the sort that just required a little code cleanup on my side. Anyway, the current fix makes TS 2.4 usable, so it's back to work for me...

@lukescott
Copy link

lukescott commented Jun 29, 2017

This actually broke my code. I have custom types like this:

export interface MyAction extends Action {
// ...
}

And now MyAction is no longer correct because it isn't AnyAction.

@lukescott
Copy link

lukescott commented Jun 29, 2017

Why was Reducer changed? If you have an excess property a new type should be defined. That's the point of strict typing. With the example of the first post you can do something like this:

import AnyAction from "redux"

const rootState: RootState = rootReducer(undefined, {
  type: 'ADD_TODO',
  text: 'test',
} as AnyAction) // cast to new type in PR `AnyAction`

With Reducer changed custom types are failing.

@jimsugg
Copy link

jimsugg commented Jun 29, 2017

@lukescott - Just as an experiment, I suggest you try suggestion from @morlay to see if that works for you:

export type Reducer<S, A extends Action = Action> = (state: S, action: A) => S;

In my test, this also fixes the issue with TS 2.4. So this might be a better solution than the one than introduces AnyAction.

@lukescott
Copy link

Yep that works. This works as well:

export type Reducer<S> = (state: S, action: Action) => S;

@jimsugg
Copy link

jimsugg commented Jun 29, 2017

@lukescott - To reiterate why this is happening (you asked why Reducer was changed) - combineReducers causes errors in Typescript 2.4 with the current definition of Reducer.

@lukescott
Copy link

lukescott commented Jun 29, 2017

@jimsugg Looks like all my issues were TypeScript 2.4, and some fumbling on my part. AnyAction does work. Although as you said, the above solution you mentioned may work better. I also found it works without the , A extends Action = Action part as well.

@blocka
Copy link

blocka commented Jul 2, 2017

Is there anyway to take advantage of this now, without waiting for it to be shipped? Or do I have to downgrade TS?

@aikoven
Copy link
Collaborator

aikoven commented Jul 2, 2017

@blocka You can use this: #2182 (comment)

@ahstro
Copy link
Contributor

ahstro commented Jul 3, 2017

Could this not warrant a patch release? It's basically the only change since 3.7.1, so wouldn't have to worry about other changes.

@coffeexy
Copy link

coffeexy commented Jul 3, 2017

mark

@jimsugg
Copy link

jimsugg commented Jul 3, 2017

I add my request for a quick rollout of this fix. This is a blocking issue for Redux users wanting to move to TS 2.4.

@aikoven
Copy link
Collaborator

aikoven commented Jul 3, 2017

I guess this is a non-breaking change and can be released as a patch.

@toranb
Copy link

toranb commented Jul 10, 2017

@aikoven any idea when this new type def will be released? Is it safe to assume it will show up in v3.7.2?

@aikoven
Copy link
Collaborator

aikoven commented Jul 11, 2017

cc. @timdorr

@timdorr
Copy link
Member

timdorr commented Jul 13, 2017

OK, 3.7.2 is out now.

@sbuzonas
Copy link

sbuzonas commented Jul 15, 2017

I'm not incredibly familiar with how to cope with this change. Would someone that is familiar with what has been done here be kind enough to take a look at this broken build? This is IRT the reference from DefinitelyTyped/DefinitelyTyped#17777

Error:

ERROR: 23:57  expect  TypeScript compile error: Argument of type '{ router: (state?: RouterState | undefined, action?: RouterAction | undefined) => RouterState; }' is not assignable to parameter of type 'ReducersMapObject'.
  Property 'router' is incompatible with index signature.
    Type '(state?: RouterState | undefined, action?: RouterAction | undefined) => RouterState' is not assignable to type 'Reducer<any>'.
      Types of parameters 'action' and 'action' are incompatible.
        Type 'AnyAction' is not assignable to type 'RouterAction | undefined'.
          Type 'AnyAction' is not assignable to type 'RouterAction'.
            Property 'payload' is missing in type 'AnyAction'.
The following packages had errors: react-router-redux
Error: There was a test failure.
    at Object.<anonymous> (/Users/sbuzonas/Projects/typescript/DefinitelyTyped/node_modules/types-publisher/src/tester/test-runner.ts:93:9)
    at Generator.next (<anonymous>)
    at fulfilled (/Users/sbuzonas/Projects/typescript/DefinitelyTyped/node_modules/types-publisher/bin/tester/test-runner.js:4:58)

The RouterAction interface is defined here.

The test that is failing is located here.

@lukescott
Copy link

@sbuzonas edit index.d.ts of redux in node modules and try the alternate Reducer definition that doesn't use AnyAction and see if that solves your issue. If it does it may need to be patched again.

The AnyAction worked for me, but it may not work in all cases.

@huy-nguyen
Copy link

@sbuzonas There's a pending PR for react-router-redux in DefinitelyTyped that tries to fix the problem. Can you look at it and comment on whether that will fix your problem?

@sbuzonas
Copy link

sbuzonas commented Jul 16, 2017

@lukescott The alternative does work, the problem is on master the typescript version is 1.8 and fails to pass the test. The changes on the next branch are along those lines and work as expected, but cannot work with such an earlier version of typescript here on master.

@huy-nguyen That PR would work, but it sacrifices the safety of type checking. It would allow a circumstance where you send an incorrect action to the wrong reducer.

Edit: Second thought, that is exactly how the code behaves. All actions are passed to the reducer, and the reducer simply returns an untouched state if it isn't the expected action.

seantcoyote pushed a commit to seantcoyote/redux that referenced this pull request Jan 14, 2018
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.

None yet