feat(store): use variadic tuple types for createSelector #3023

david-shortman · 2021-05-18T02:47:23Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Types were manually created for set numbers of arguments.

Completes #2715 (I don't see any reducer code that still needs variadic typing?)

What is the new behavior?

createSelector stays strongly typed for an infinite number of arguments.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Some small typing adjustments.

david-shortman · 2021-05-18T02:48:56Z

modules/store/src/selector.ts

+/**
+ * @deprecated Selectors with props are deprecated, for more info see {@link https://github.com/ngrx/platform/issues/2980 Github Issue}
+ */
+export function createSelector<State, Props, S1, Result>(


left the existing definitions for createSelector when creating selectors with props since it's deprecated and will eventually be removed

ngrxbot · 2021-05-18T02:57:15Z

Preview docs changes for 41df527 at https://previews.ngrx.io/pr3023-41df527a/

modules/store/spec/selector.spec.ts

modules/store/src/selector.ts

david-shortman · 2021-05-20T02:32:28Z

The example-app tests are failing on circleci and yet "it works on my machine"

the error complains about selectAll being the wrong type MemoizedSelectorWithProps and yet:

 FAIL   Example App  projects/example-app/src/app/books/reducers/books.reducer.spec.ts
  ● Test suite failed to run

    modules/entity/src/state_selectors.ts:27:9 - error TS2322: Type 'MemoizedSelectorWithProps<any, unknown, any, DefaultProjectorFn<any>>' is not assignable to type '(state: any) => T[]'.

    27         selectAll,
               ~~~~~~~~~

      modules/entity/src/models.ts:83:3
        83   selectAll: (state: V) => T[];
             ~~~~~~~~~
        The expected type comes from property 'selectAll' which is declared here on type 'EntitySelectors<T, any>'
    modules/entity/src/state_selectors.ts:35:7 - error TS2322: Type 'MemoizedSelectorWithProps<any, unknown, any, DefaultProjectorFn<any>>' is not assignable to type '(state: any) => T[]'.

    35       selectAll: createSelector(selectState, selectAll),
             ~~~~~~~~~

timdeschryver · 2021-05-20T06:23:03Z

Does that error disappears if you remove the deprecated selector with props?
The error message in the CI, indicates that that type of selector is used.

modules/store/spec/selector.spec.ts

david-shortman · 2021-06-04T01:33:47Z

Since there can be minor type adjustments that are breaking, it may be best to pair this with a pr that completely removes SelectorWithProps to finish off dealing with selectors for v13: #3035.

david-shortman · 2021-06-05T15:42:05Z

So it all works now, but in some circumstances (as evidenced by the few changes to other files) the overloads are not properly recognized if the projector functions have explicitly typed parameters. Not sure why and if anyone has advanced TS knowledge to avoid this as to not make this a breaking change, that'd be litty.

brandonroberts · 2021-07-05T20:27:56Z

Is this dependent on selectors with props being removed? If so, I say we close it until then.

david-shortman · 2021-07-06T20:04:48Z

Is this dependent on selectors with props being removed? If so, I say we close it until then.

This is not dependent on selectors with props being removed.

timdeschryver · 2021-07-06T21:10:02Z

Since the types in some tests are changed, I think this will lead to some breaking changes?
If that's the case, this PR should also have to wait for v13.

david-shortman · 2021-07-07T01:50:46Z

Since the types in some tests are changed, I think this will lead to some breaking changes?
If that's the case, this PR should also have to wait for v13.

Right now it's a breaking change, but I'm pretty sure there's something that could be done either with the types or with a Typescript issue that would cause the overloads to not be improperly interpreted... if someone smarter than me could find a solution 😅.

david-shortman · 2021-07-10T03:13:07Z

@timdeschryver This is a breaking change because you cannot provide a specific set of generic type arguments to createSelector anymore.

So createSelector<State, S1, S2, S3, Result>(...) is invalid now, and instead, if someone were to use explicit generic type arguments, they would have to do this: `createSelector<State, [S1, S2, S3], Result>.

david-shortman · 2021-07-10T03:15:20Z

projects/data-example-app/src/state/story.selectors.ts

@@ -1,7 +1,7 @@
 import { createSelector } from '@ngrx/store';
 import { Stories, Story } from './story';

-export const selectStories = createSelector<Story[], Story[], Story[][]>(
+export const selectStories = createSelector<Story[], [Story[]], Story[][]>(


Explicitly provided generic arguments must take the format <State, [S1, S2, Etc], Result>

MaximSagan · 2021-10-22T06:37:38Z

Since the types in some tests are changed, I think this will lead to some breaking changes? If that's the case, this PR should also have to wait for v13.

@timdeschryver Seems like v13 of Angular is approaching, and it seems since the last NGRX v12 release you've started deprecating features in master, so I guess we're gearing up for NGRX v13 too?
Could we get this PR merged into master now?

timdeschryver · 2021-10-22T12:36:42Z

@MaximSagan we're not there yet.
We will try to follow the Angular releases, so we can have v13 released shortly hereafter.

david-shortman · 2021-11-05T13:24:12Z

@MaximSagan we're not there yet.
We will try to follow the Angular releases, so we can have v13 released shortly hereafter.

@timdeschryver Is this planned for v13? I noticed it wasn't included for the first beta.

david-shortman · 2021-11-09T00:50:43Z

@MaximSagan we're not there yet.
We will try to follow the Angular releases, so we can have v13 released shortly hereafter.

@brandonroberts Wondering if there is a blocker for this being included in v13?

brandonroberts · 2021-11-09T00:52:16Z

@david-shortman no blocker. I wasn't sure if this was ready to be merged yet

david-shortman · 2021-11-09T00:52:53Z

@david-shortman no blocker. I wasn't sure if this was ready to be merged yet

@brandonroberts It should be. I'm not sure why it doesn't have any reviews yet, but it is ready.

timdeschryver · 2021-11-09T14:09:14Z

Sorry for my late response, I can take a look at it later this week and provide some feedback then.

david-shortman · 2021-11-09T14:11:57Z

Sorry for my late response, I can take a look at it later this week and provide some feedback then.

@timdeschryver thanks! Also, I've created a PR dependent on this one, #3223, which offers a gentle path through depreciation to move towards a strongly-typed projector method on selectors.

timdeschryver

👏👏 @david-shortman

This looks good to me.
We could also provide a schematic that provides a fix for the breaking change, e.g.

BEFORE:

createSelector<Story[], Story[], Story[][]>

AFTER:

//        needs to be a tuple 👇
createSelector<Story[], [ Story[] ] , Story[][]>

david-shortman · 2021-11-10T19:53:32Z

👏👏 @david-shortman

This looks good to me.

We could also provide a schematic that provides a fix for the breaking change, e.g.

BEFORE:
createSelector<Story[], Story[], Story[][]>
AFTER:
//        needs to be a tuple 👇

createSelector<Story[], [ Story[] ] , Story[][]>

I haven't written a schematic before, but it could be fun to try.

Should I go for that in a separate PR?

brandonroberts · 2021-11-10T21:35:19Z

@david-shortman @timdeschryver is the tuple only required when you strictly define all the types with createSelector?

david-shortman · 2021-11-10T23:06:53Z

@david-shortman @timdeschryver is the tuple only required when you strictly define all the types with createSelector?

The generic arguments are inferred, so that you can use const inferred = createSelector(() => 1, () => 2, (one, two) => 3).

When manually specifying the generic arguments, you have to specify the selector's list of selector return values, like const strictlySpecified = createSelector<State, [1, 2], 3>(...)

markostanimirovic

Great work David! 🥇

Btw, it would be great to add type tests for selectors :)

markostanimirovic · 2021-11-11T01:30:46Z

modules/store/src/selector.ts

-export function createSelector<State, S1, Result>(
-  s1: Selector<State, S1>,
-  projector: (s1: S1) => Result
+export function createSelector<State, S extends unknown[], Result>(


It would be great to give a meaningful name to the S generic because it's not clear at first what is its purpose. The name could be: Slices

markostanimirovic · 2021-11-11T01:31:07Z

modules/store/src/selector.ts

-  s1: Selector<State, S1>,
-  projector: (s1: S1) => Result
+export function createSelector<State, S extends unknown[], Result>(
+  ...args: [...Selector<State, unknown>[], unknown] &


nice hack 👀

🙇‍♂️

markostanimirovic · 2021-11-11T01:31:54Z

modules/store/src/selector.ts

    props: Props
  ) => Result
 ): MemoizedSelectorWithProps<State, Props, Result>;
-export function createSelector<State, S1, S2, S3, S4, S5, S6, S7, Result>(
+
+export function createSelector<State, S extends unknown[], Result>(


the same here: S => Slices

david-shortman · 2021-11-11T05:52:04Z

Great work David! 🥇

Btw, it would be great to add type tests for selectors :)

I think ts-snippet can only validate the return type of the function, not whether the function inferred the parameters correctly.

In #3223, the return type is validated in type checks since the Slices are used in MemoziedSelector.

markostanimirovic

LGTM 👍

markostanimirovic · 2021-11-11T12:30:42Z

Changed PR type from refactor to feature, for two reasons:

Breaking change with types
The createSelector function can now accept any number of input selectors.

brandonroberts · 2021-11-11T12:35:36Z

🥳

david-shortman commented May 18, 2021

View reviewed changes

david-shortman commented May 20, 2021

View reviewed changes

modules/store/spec/selector.spec.ts Outdated Show resolved Hide resolved

david-shortman commented May 20, 2021

View reviewed changes

modules/store/src/selector.ts Outdated Show resolved Hide resolved

david-shortman marked this pull request as ready for review May 20, 2021 02:22

david-shortman force-pushed the create-selector-type-update branch from 0dc1ca4 to c53a8d4 Compare June 4, 2021 01:19

david-shortman commented Jun 4, 2021

View reviewed changes

modules/store/spec/selector.spec.ts Show resolved Hide resolved

david-shortman mentioned this pull request Jun 4, 2021

No type-safety for selector's projector function #2317

Closed

david-shortman force-pushed the create-selector-type-update branch from 985578f to 53fd1cd Compare July 2, 2021 00:47

david-shortman mentioned this pull request Jul 2, 2021

refactor(store) Remove deprecated SelectorWithProps #3061

Closed

3 tasks

david-shortman added 6 commits July 9, 2021 22:15

refactor(store) used variadic tuple types for createSelector

5c50098

derive State properly

4ab4c08

corrected override

064b5e6

fixed implicit any with explicit any

2d1f421

removed explicit types that broke typescript's brain

ccc1dd1

removed unneeded explicit any

3265126

david-shortman force-pushed the create-selector-type-update branch 2 times, most recently from 08b00f3 to 3265126 Compare July 10, 2021 03:09

Update story.selectors.ts

4cabfa1

david-shortman commented Jul 10, 2021

View reviewed changes

MaximSagan mentioned this pull request Oct 22, 2021

createSelector should return MemoizedSelector<State, Result, ProjectorFn> with fully-typed ProjectorFn to assist in unit-testing selectors #3196

Closed

david-shortman force-pushed the create-selector-type-update branch from dd49aef to 4cabfa1 Compare November 9, 2021 06:41

david-shortman mentioned this pull request Nov 9, 2021

feat(store) add overload for projector to inform about incorrect usage #3223

Closed

3 tasks

brandonroberts requested review from timdeschryver and markostanimirovic November 10, 2021 13:25

timdeschryver approved these changes Nov 10, 2021

View reviewed changes

markostanimirovic reviewed Nov 11, 2021

View reviewed changes

Merge branch 'master' into create-selector-type-update

490a1ee

updated S -> Slices

41df527

david-shortman requested a review from markostanimirovic November 11, 2021 06:19

markostanimirovic approved these changes Nov 11, 2021

View reviewed changes

markostanimirovic changed the title ~~refactor(store) used variadic tuple types for createSelector~~ feat(store): use variadic tuple types for createSelector Nov 11, 2021

brandonroberts merged commit 367d9b4 into ngrx:master Nov 11, 2021

timdeschryver added the Breaking Change label Nov 11, 2021

david-shortman deleted the create-selector-type-update branch November 11, 2021 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(store): use variadic tuple types for createSelector #3023

feat(store): use variadic tuple types for createSelector #3023

david-shortman commented May 18, 2021 •

edited by markostanimirovic

Loading

david-shortman May 18, 2021 •

edited

Loading

ngrxbot commented May 18, 2021 •

edited

Loading

david-shortman commented May 20, 2021 •

edited

Loading

timdeschryver commented May 20, 2021

david-shortman commented Jun 4, 2021

david-shortman commented Jun 5, 2021

brandonroberts commented Jul 5, 2021

david-shortman commented Jul 6, 2021

timdeschryver commented Jul 6, 2021

david-shortman commented Jul 7, 2021

david-shortman commented Jul 10, 2021

david-shortman Jul 10, 2021

MaximSagan commented Oct 22, 2021

timdeschryver commented Oct 22, 2021

david-shortman commented Nov 5, 2021

david-shortman commented Nov 9, 2021

brandonroberts commented Nov 9, 2021

david-shortman commented Nov 9, 2021 •

edited

Loading

timdeschryver commented Nov 9, 2021

david-shortman commented Nov 9, 2021

timdeschryver left a comment •

edited

Loading

david-shortman commented Nov 10, 2021

brandonroberts commented Nov 10, 2021

david-shortman commented Nov 10, 2021 •

edited

Loading

markostanimirovic left a comment

markostanimirovic Nov 11, 2021 •

edited

Loading

markostanimirovic Nov 11, 2021

david-shortman Nov 11, 2021

markostanimirovic Nov 11, 2021

david-shortman commented Nov 11, 2021 •

edited

Loading

markostanimirovic left a comment

markostanimirovic commented Nov 11, 2021

brandonroberts commented Nov 11, 2021

feat(store): use variadic tuple types for createSelector #3023

feat(store): use variadic tuple types for createSelector #3023

Conversation

david-shortman commented May 18, 2021 • edited by markostanimirovic Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

david-shortman May 18, 2021 • edited Loading

Choose a reason for hiding this comment

ngrxbot commented May 18, 2021 • edited Loading

david-shortman commented May 20, 2021 • edited Loading

timdeschryver commented May 20, 2021

david-shortman commented Jun 4, 2021

david-shortman commented Jun 5, 2021

brandonroberts commented Jul 5, 2021

david-shortman commented Jul 6, 2021

timdeschryver commented Jul 6, 2021

david-shortman commented Jul 7, 2021

david-shortman commented Jul 10, 2021

david-shortman Jul 10, 2021

Choose a reason for hiding this comment

MaximSagan commented Oct 22, 2021

timdeschryver commented Oct 22, 2021

david-shortman commented Nov 5, 2021

david-shortman commented Nov 9, 2021

brandonroberts commented Nov 9, 2021

david-shortman commented Nov 9, 2021 • edited Loading

timdeschryver commented Nov 9, 2021

david-shortman commented Nov 9, 2021

timdeschryver left a comment • edited Loading

Choose a reason for hiding this comment

david-shortman commented Nov 10, 2021

brandonroberts commented Nov 10, 2021

david-shortman commented Nov 10, 2021 • edited Loading

markostanimirovic left a comment

Choose a reason for hiding this comment

markostanimirovic Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

markostanimirovic Nov 11, 2021

Choose a reason for hiding this comment

david-shortman Nov 11, 2021

Choose a reason for hiding this comment

markostanimirovic Nov 11, 2021

Choose a reason for hiding this comment

david-shortman commented Nov 11, 2021 • edited Loading

markostanimirovic left a comment

Choose a reason for hiding this comment

markostanimirovic commented Nov 11, 2021

brandonroberts commented Nov 11, 2021

david-shortman commented May 18, 2021 •

edited by markostanimirovic

Loading

david-shortman May 18, 2021 •

edited

Loading

ngrxbot commented May 18, 2021 •

edited

Loading

david-shortman commented May 20, 2021 •

edited

Loading

david-shortman commented Nov 9, 2021 •

edited

Loading

timdeschryver left a comment •

edited

Loading

david-shortman commented Nov 10, 2021 •

edited

Loading

markostanimirovic Nov 11, 2021 •

edited

Loading

david-shortman commented Nov 11, 2021 •

edited

Loading