TypeScript definitions allow a reducer that doesn't handle all possible actions

[Mossop]

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

If the current behaviour is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar.

I'm using TypeScript and hit a situation where I managed to register my main reducer as only receiving my specific types of actions. This is a simplified version though somehow in my real case I managed to make the reducer not even need to accept undefined for the state: https://gist.github.com/Mossop/2871372747e55e0fedea64eb649067e6.

This compiles correctly with TypeScript because TypeScript thinks I have handled all possible actions, and returned a valid state however this is incorrect. My reducer will actually receive other actions such as the INIT action that redux sends when the store is first created. When this reaches my reducer it just returns undefined and now my store is in an unexpected state.

What is the expected behavior?

Obviously I can fix this by just always putting return state at the end of any reducers, but I think that TypeScript should catch this case, createStore should only accept a reducer of the form (state: S | undefined, action: Action) => S

Which versions of Redux, and which browser and OS are affected by this issue? Did this work in previous versions of Redux?

I'm testing with 4.0.4, haven't tried any other versions.

[nickserv]

Redux has recently been ported to TypeScript which may fix this issue, though it hasn't been released yet. Maybe you could try building it from source.

[timdorr]

This is because we coerce the INIT and REPLACE actions to your Action type:

redux/src/createStore.ts

Line 340 in 25f8a6c

dispatch({ type: ActionTypes.INIT } as A)

redux/src/createStore.ts

Line 286 in 25f8a6c

dispatch({ type: ActionTypes.REPLACE } as A)

Perhaps we can make something like reducer(state: State, action: ReduxActions<MyActions>) => State that will include the internal actions?

[Mazuh]

@timdorr could you follow up the discussion that @markerikson started with me at #3582 ?

I recently started to study TS and I always use Redux, so I got interested in this issue

[timdorr]

So, we basically lie and say these special actions with randomly-generated types are actually set with the types you define. Your own Action type (which extends our Action type) is going to specify the the type field to a known set of values (essentially, an enum). So our casting of the internal action types will create a scenario where TS will pass the types, but produce incorrect code.

More specifically, in @Mossop's example, that would produce an error if used with combineReducers when it attempts to probe an unknown action. And it would produce a state of undefined when creating the store (from the INIT action), which may be fine, but is probably unexpected and will likely cause Cannot read property 'foo' of undefined errors.

So, the core problem is we're preventing TS from doing its job effectively and creating the potential for bugs to go unnoticed.

[Mazuh]

@timdorr and that solution I implemented isn't really an option (available on current design)?

[Mazuh]

I'm not sure of how it's possíble for the user to implement a typed-safe reducer without somehow being exposed to all possible actions.

[timdorr]

I don't think you were totally wrong, but I think I was wrong about having a separate ReduxAction. We should keep our types as similar as possible.

Here's a better example of what I'm thinking of.

Note that the return type of the reducer fails in that case. The previous example will still fail to catch the bug, but it's fully BC with existing code that way. At least you now have the option to add some more safety.

[cellog]

Step 1: can we make a PR with a failing typescript test that can be iterated on to solve this?

[r3dm1ke]

Hi! I am trying to write a failing typescript test that we can use for this, but I ran into errors with running jest: it will complain about not being able to parse ts code, for example:

import { createStore, combineReducers } from '..';
           ^

    SyntaxError: Unexpected token {

when running npm run test. Is that a well-know issue that has a solution, or is it a local problem on my end? I am running this on Windows, if it matters.

[jsnajdr]

We ran into a similar (or exactly the same?) issue where the reducer type incorrectly constrains the set of allowed actions.

The Usage with TypeScript guide recommends typing the action parameter as the union of action types the reducer specifically reacts to:

function chatReducer(state = initialState, action: ChatActionTypes): ChatState {
  switch (action.type) {
    case SEND_MESSAGE:
      return ...;
    case DELETE_MESSAGE:
      return ...;
    default:
      return state;
  }
}

But that's too constraining. A Redux reducer must expect and correctly handle any action of the general-purpose Action type, i.e., an object with a type field.

TypeScript then refuses to compile 100% valid code that calls the reducer:

chatReducer(undefined, { type: 'FOO' });
createStore(chatReducer).dispatch({ type: 'FOO' });

Both these statements fail with:

Type '"FOO"' is not assignable to type '"SEND_MESSAGE" | "DELETE_MESSAGE"'

If I understand the current types correctly, they are great for providing autocomplete hints, but are not that good at the main job of type checking, i.e., checking correctness of the compiled program.

[bdwain]

EDIT: I think there's a better way. Described in my next comment.

Ageeed with @jsnajdr. We should not be sacrificing correctness for convenience. Especially when casting actions explicitly is not actually so inconvenient. And the solutions that deal with the built in redux actions specifically ignore the fact that my reducer needs to handle any arbitrary action that gets dispatched, not just the built in ones.

Unfortunately I don’t think it’s currently possible with typescript to have a type for the actions that allows for type inference from a switch statement. Ideally there is an enhancement in the future that allows these types to be expressed. I’ve seen a bunch of issues on there talking about subtraction types and negation types that seem like they could help with this. But those issues have all been around for 3+ years, so without anything concrete on the horizon, I feel like there’s only one path forward for now that will actually lead to correct types, and that is forcing the type of action to be AnyAction and requiring people to cast their actions to the correct type. That sounds like it would be a breaking change, but the longer the types are left as broken the harder it will be for people to migrate when the change actually is made.

[bdwain]

Actually @laszlopandy had a better idea that I think solves this issue pretty well. Just make up a fake Action type that the reducer must handle, and the default case will be how it gets covered.

export interface Reducer<S, A extends Action = AnyAction> {
  (state: S | undefined, action: A | Action<'@@REDUX/HANDLE_DEFAULT_CASE'>): S;
}

the only way to satisfy that fake Action type is to handle the default case or to handle it explicitly (which, if you do, then 🤷)

it's a little weird as an implementation but I see no downsides from the perspective of someone using redux unless they inspect the redux types and get confused about the fake action. but a good comment will help that.

[jsnajdr]

Just make up a fake Action type that the reducer must handle, and the default case will be how it gets covered.

That forces the reducer author to cover the default case, but calling the reducer with arbitrary action:

chatReducer(undefined, { type: 'FOO' });
createStore(chatReducer).dispatch({ type: 'FOO' });

will continue to fail the type check, won't it?

The A | Action<'@@REDUX/HANDLE_DEFAULT_CASE'> type avoids some practical issues (missing autocomplete, default case not handled, ...), but it's still not a truthful description of what the type of the reducer action parameter actually is.

[bdwain]

oh that's true. It would fail on the first line, but not the second, as that expects the same reducer type that we'd be changing. This is also true with the current implementation.

So I guess my original comment is still accurate, in that there is no perfect solution that can be implemented with the current typescript, as it's not possible to correctly type the reducer and get type inference.

I am less worried about the types being off here, as they won't lead to incorrect reducers, just maybe a requirement that you cast your action if you call your reducer directly and it's not an expected type. But calling your reducer directly is pretty rare, as it's mostly done via combineReducers or similar helpers.

The flip side is that having 100% correct types but losing type inference has risks to correctness also, in that you could accidentally skip handling a known action and it would not trigger a type error.

[jsnajdr]

you could accidentally skip handling a known action and it would not trigger a type error.

If the reducer's switch statement doesn't handle a certain action type and simply returns the state unchanged, how can that become a type error? The types of the reducer inputs, outputs and everything in between are still OK.

I agree that there is no perfect solution and some tradeoff needs to be chosen.

[bdwain]

It wouldn’t be a type error. I just meant that it’s a type of bug that can be prevented by specifying the action types up front. So there are trade offs like you said

[cellog]

Is this really an issue? The app will dramatically break on the first action that isn't supported. If you have any tests whatsoever they will catch it. Typescript can't solve everything. I just put a rrturn state at the bottom of every reducer. VS Code shows it as not being a reachable line, but it doesn't cause an error.

[bdwain]

For a widely used library like redux, I think yes it's worth it. Someone may accidentally (or on purpose) not test their change before deploying it, and this could prevent a bug.

I also think there's no downsides to adding the | Action<'@@REDUX/HANDLE_DEFAULT_CASE'> vs not adding it, so it seems like a nobrainer (you get more type safety, lose no type inference or convenience, and you already needed to cast actions you pass directly anyway)