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

takeLatest and similar should typecheck pattern parameter. #1543

Closed
BenLorantfy opened this issue Jul 26, 2018 · 7 comments · May be fixed by #1984
Closed

takeLatest and similar should typecheck pattern parameter. #1543

BenLorantfy opened this issue Jul 26, 2018 · 7 comments · May be fixed by #1984

Comments

@BenLorantfy
Copy link

This isn't a bug report per se, but more of a suggestion on how to improve the typings.

  type LoadDataAction = { type: 'LOAD_DATA' }
  yield takeLatest<LoadDataAction>('blablabla', loadData);

As a user, I'd expect the above code to throw an error, because 'blablabla' is not equal to 'LOAD_DATA'. This code should be fine:

  type LoadDataAction = { type: 'LOAD_DATA' }
  yield takeLatest<LoadDataAction>('LOAD_DATA', loadData);

This should work similarly for takeEvery, take, etc.

@BenLorantfy
Copy link
Author

This is something I'd be willing to open a PR for if wanted.

@feichao93
Copy link
Member

PRs are welcome. I would like to see how you implement the typecheck for takeEvery. It is complex since takeEvery has several overloading.

@Andarist
Copy link
Member

Andarist commented Sep 3, 2018

@BenLorantfy will you be able to prepare a PR for this? if not we might close this issue because we are not actively working on typings, this is a community effort, not ours.

@aikoven is it possible to type check this?

@BenLorantfy
Copy link
Author

I took a quick look but it looks pretty complicated and I’m not sure where to start. In theory it’s possible (using generics) but I’m not sure how to fit it into the existing typings. I think this would be a useful type check though, so I don’t think this should be closed.

@aikoven
Copy link
Contributor

aikoven commented Sep 5, 2018

This indeed looks pretty complicated, since you can use many different things as pattern: predicates, action creators, action types and arrays of those.

This is how ActionPattern type is defined:

export type ActionType = string | number | symbol;

export type StringableActionCreator<A extends Action = Action> = {
  (...args: any[]): A;
  toString(): string;
};

export type ActionSubPattern<Guard extends Action = Action> =
  | GuardPredicate<Guard, Action>
  | StringableActionCreator<Guard>
  | Predicate<Action>
  | ActionType

export type ActionPattern<Guard extends Action = Action> =
  | ActionSubPattern<Guard>
  | ActionSubPattern<Guard>[];

The path we're interested in is ActionPattern<T> -> ActionSubPattern<T> -> ActionType, and ActionType is not parametric.

We could parametrize it like this:

type ActionType<A extends Action = Action> = A['type'];

But unfortunately in Redux typings the type of Action.type is any, so this would effectively cast ActionPattern to any when used without a type parameter.

@fyodore82
Copy link

if takeLatest is invoked with type argument which extends Action (any object, which has type property), Guard type argument of ActionSubPattern will be properly typed.

So if change ActionSubPattern to something like

export type ActionSubPattern<Guard extends Action = Action> =
  | GuardPredicate<Guard, Action>
  | StringableActionCreator<Guard>
  | Predicate<Action>
  | Guard['type']

The result will be as expected. Below code will produce type error

type LoadDataAction = { type: 'LOAD_DATA' }
yield takeLatest<LoadDataAction>('blablabla', loadData);

The only this which confuse me is form of takeEvery<P extends ActionPattern>. I'll look at why there are two different forms of takeEvery exists and then propose change of typing.

@neurosnap
Copy link
Member

Closing due to inactivity. If this is still something worth discussing please reply and I will reopen to help. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants