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

RFC: Capture the type of a selector's projector function in the MemoizedSelector type #1908

Closed
MikeRyanDev opened this issue Jun 3, 2019 · 6 comments
Labels
Accepting PRs community watch Someone from the community is working this issue/PR enhancement Project: Store
Projects

Comments

@MikeRyanDev
Copy link
Member

When testing a memoized selector's projector function we default the type of the projector to be any. This makes it difficult to catch type changes of the projector function in unit tests.

I recommend the type of MemoizedSelector be extended to have a third type parameter that captures the type of the projector function:

export interface MemoizedSelector<State, Result, ProjectorFn>
  extends Selector<State, Result> {
  release(): void;
  projector: ProjectorFn;
  setResult: (result?: Result) => void;
}

Describe any alternatives/workarounds you're currently using

Live with the lack of type safety. 😄

Other information:

If accepted, I would be willing to submit a PR for this feature

[X] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

@brandonroberts brandonroberts added Need Discussion Request For Comment needs input 8.x and removed 8.x labels Jun 3, 2019
@brandonroberts brandonroberts added this to To Do in NgRx 8 Jun 3, 2019
@alex-okrushko
Copy link
Member

alex-okrushko commented Jun 3, 2019

Having the projector to be any bothered me as well. Would this be a better solution?

export interface MemoizedSelector<State, Result>
  extends Selector<State, Result> {
  release(): void;
  projector: (...args: any[]) => Result;
  setResult: (result?: Result) => void;
}

I'd rather not add third parameter to the MemoizedSelector (that doesn't have a default value) - it would break the world for me.

@brandonroberts
Copy link
Member

I would propose adding a default value of AnyFn for ProjectorFn for version 8, and revisit the breaking change for version 9.

@MikeRyanDev
Copy link
Member Author

@brandonroberts, @alex-okrushko To follow on, I would say that the interface provides a default value for the new third generic parameter:

export interface MemoizedSelector<State, Result, ProjectorFn = AnyFn>
  extends Selector<State, Result> {
  release(): void;
  projector: ProjectorFn;
  setResult: (result?: Result) => void;
}

but using inference from createSelector you get a more specific type for the third parameter.

@alex-okrushko
Copy link
Member

This looks like a non-breaking change (unless some code erroneously was relying on a different return type).
We could change to the following:

export type DefaultProjectorFn<T> = (...args: any[]) => T;

export interface MemoizedSelector<State, Result, ProjectorFn = DefaultProjectorFn<Result>>
  extends Selector<State, Result> {
  release(): void;
  projector: ProjectorFn;
  setResult: (result?: Result) => void;
}

WDYT?

@brandonroberts
Copy link
Member

👍

@jasonhodges
Copy link
Contributor

I will take this one 👍

@brandonroberts brandonroberts added the community watch Someone from the community is working this issue/PR label Jun 4, 2019
@brandonroberts brandonroberts moved this from To Do to In Progress in NgRx 8 Jun 5, 2019
NgRx 8 automation moved this from In Progress to Done Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepting PRs community watch Someone from the community is working this issue/PR enhancement Project: Store
Projects
No open projects
NgRx 8
  
Done
Development

No branches or pull requests

4 participants