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

Reducers are not typesafe and behave incorrectly in the default case of the switch #3792

Closed
bdwain opened this issue Jun 6, 2020 · 2 comments

Comments

@bdwain
Copy link

bdwain commented Jun 6, 2020

Prior Issues

none

What is the current behavior?

The reducer type is not type safe. It assumes you never reach your default case in the switch statement.

export type Reducer<S = any, A extends Action = AnyAction> = (
  state: S | undefined,
  action: A
) => S

By limiting the action parameter to the subset of types your reducer cares about, we introduce a bug where the inferred type of action in the default case is never. This is because when you do a union type in typescript, it implicitly adds | never at the end;

Steps to Reproduce

type MyActionTypes = ActionType1 | ActionType2; //implicitly ActionType1 | ActionType2 | never

const MyReducer: Reducer<MyState, MyActionTypes> = (state = initState, action) => {
  switch(action.type){
    case actionTypes.TYPE_1:
      return handleType1(state, action); //infers that action is ActionType1

    case actionTypes.TYPE_2:
      return handleType2(state, action); //infers that action is ActionType2
    
	default:
	  return action; //infers that action type is never, so it does not complain that the return type is invalid because it assumes this will never happen anyway
  } 
};

What is the expected behavior?

Have a type error if you return action from the default case

Environment Details

n/a

I found the original discussion that lead to this change, and while I get the convenience benefit of constraining the types to allow typescript to infer the action type for you, that comes with the cost of types not being correct and I think that's not the right tradeoff to make. I also get that this is a big breaking change that is difficult to make, but this seems like a really easy bug to make accidentally, and if someone heavily relies on typescript to catch type errors (which they should be able to do), this can affect them.

I think at worst, forcing you to do something like this is not a large cost to pay for correctness.

const MyReducer: Reducer<MyState> = (state = initState, action) => {
  switch(action.type){
    case actionTypes.TYPE_1:
      return handleType1(state, action as ActionType1);

    case actionTypes.TYPE_2:
      return handleType2(state, action as ActionType2);
    
	default:
	  return action; //TYPE ERROR
  } 
};

I'm also not sure if it's possible, but I think ideally, we could get the benefits of type inference without losing type safety. If there was some sort of "difference" operator in typescript, which was similar to unions and intersections but said A ~ B is types that are A and not B, then we could do something like this

export type Reducer<S = any, A extends Action = AnyAction> = (
  state: S | undefined,
  action: A | (AnyAction ~ A)
) => S

I was hoping that Exclude<A, B> would be the difference type I was looking for, but it doesn't seem to work that way.

I'm not sure if the difference type is possible, and even if it is, if typescript can do the type inference we get currently while also realizing that in the default case, action is not never. But even if not, I think this is something that should be addressed.

@bdwain
Copy link
Author

bdwain commented Jun 6, 2020

Update: It also doesn't complain if you forget a default statement altogether, which is probably more likely than returning the action

@timdorr
Copy link
Member

timdorr commented Jun 7, 2020

Duplicate of #3580

@timdorr timdorr marked this as a duplicate of #3580 Jun 7, 2020
@timdorr timdorr closed this as completed Jun 7, 2020
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

No branches or pull requests

2 participants