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

refactor(store) used variadic tuple types for createSelector #2905

Conversation

david-shortman
Copy link
Contributor

@david-shortman david-shortman commented Jan 31, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[x] Other... Please describe: Technically this allows for typed selectors with more than 8 input selectors, so it's kinda a "feature" upgrade as well.

What is the current behavior?

Types were manually created for set numbers of arguments.

Partially completes #2715

What is the new behavior?

createSelector stays strongly typed for an infinite number of arguments.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jan 31, 2021

Preview docs changes for 9c16f81 at https://previews.ngrx.io/pr2905-9c16f81e/

@david-shortman
Copy link
Contributor Author

opening as a draft to request help

@david-shortman david-shortman reopened this Feb 3, 2021
@david-shortman david-shortman marked this pull request as draft February 3, 2021 05:13
@david-shortman
Copy link
Contributor Author

Some notes:

  • The TS 4.2 beta is required for the tuple types to work properly. This is due to the update for leading and middle rest elements in tuple types.
  • This is currently a breaking change, as the type is much stricter than before. For instance, the selector.spec.ts file in this repo was using jasmine.Spy functions for many selectors, which cannot now be interpreted properly without a type assertion
    • Am looking for feedback on this to know what the typical route is for breaking changes. Here, if we introduce a loose overload that accepts any type for the parameters, then the usefulness of the other overloads is greatly reduced.

@david-shortman david-shortman force-pushed the create-selector-type-upgrade branch from c560e68 to 484b40c Compare February 3, 2021 05:19
export function createSelector<
State,
S extends SelectorWithProps<any, any, any>[],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old definition of selectors with props specified that all input selectors had those props, but that doesn't quite make sense to me. Couldn't they be any arbitrary selectors, and the Props are merely inferred from the projector?

@david-shortman david-shortman force-pushed the create-selector-type-upgrade branch 2 times, most recently from 113095f to 9c16f81 Compare February 10, 2021 02:52
@david-shortman david-shortman force-pushed the create-selector-type-upgrade branch from 9c16f81 to f1ba3ba Compare February 20, 2021 03:04
@david-shortman
Copy link
Contributor Author

Waiting on Angular to support TS 4.2 before continuing

@brandonroberts
Copy link
Member

Let's revisit this after TS 4.2 lands in Angular

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

Successfully merging this pull request may close these issues.

3 participants