Skip to content

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

Merged
timdorr merged 1 commit into
reduxjs:masterfrom
DanielRosenwasser:fixReducer
Jun 28, 2017
Merged

timdorr merged 1 commit into
reduxjs:masterfrom
DanielRosenwasser:fixReducer

Conversation

@DanielRosenwasser

Copy link
Copy Markdown
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

jimsugg commented Jun 28, 2017

Copy link
Copy Markdown

+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

morlay commented Jun 28, 2017

Copy link
Copy Markdown

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

timdorr commented Jun 28, 2017

Copy link
Copy Markdown
Member

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

jimsugg commented Jun 28, 2017 •

Copy link
Copy Markdown

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

jimsugg commented Jun 28, 2017

Copy link
Copy Markdown

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

jimsugg commented Jun 28, 2017

Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

Thanks!

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

morlay commented Jun 29, 2017

Copy link
Copy Markdown

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

@jimsugg

jimsugg commented Jun 29, 2017 •

Copy link
Copy Markdown

@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

lukescott commented Jun 29, 2017 •

Copy link
Copy Markdown

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

lukescott commented Jun 29, 2017 •

Copy link
Copy Markdown

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

jimsugg commented Jun 29, 2017

Copy link
Copy Markdown

@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
Copy Markdown

Yep that works. This works as well:

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

@jimsugg

jimsugg commented Jun 29, 2017

Copy link
Copy Markdown

@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

lukescott commented Jun 29, 2017 •

Copy link
Copy Markdown

@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

blocka commented Jul 2, 2017

Copy link
Copy Markdown

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

@aikoven

aikoven commented Jul 2, 2017

Copy link
Copy Markdown
Collaborator

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

@ahstro

ahstro commented Jul 3, 2017

Copy link
Copy Markdown
Contributor

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

coffeexy commented Jul 3, 2017 •

Copy link
Copy Markdown

mark

@jimsugg

jimsugg commented Jul 3, 2017

Copy link
Copy Markdown

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

aikoven commented Jul 3, 2017

Copy link
Copy Markdown
Collaborator

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

@toranb

toranb commented Jul 10, 2017 •

Copy link
Copy Markdown

@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

aikoven commented Jul 11, 2017

Copy link
Copy Markdown
Collaborator

cc. @timdorr

@timdorr

timdorr commented Jul 13, 2017

Copy link
Copy Markdown
Member

OK, 3.7.2 is out now.

@sbuzonas

sbuzonas commented Jul 15, 2017 •

Copy link
Copy Markdown

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
Copy Markdown

@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
Copy Markdown

@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

sbuzonas commented Jul 16, 2017 •

Copy link
Copy Markdown

@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.