No type-safety for selector's projector function

[dzonatan]

Is there a reason why projector function does not have a type-safety for its parameters yet?
This quickly becomes a nightmare when having a lot of unit tests for complicated projectors.
I've looked into internals and found that for some reason the MemoizedSelector is using a default type DefaultProjectorFn for its projector which does not know anything about parameter types but only Result:

export function createSelector<State, S1, Result>(
  s1: Selector<State, S1>,
  projector: (s1: S1) => Result
): MemoizedSelector<State, Result>; // <-- third type is a `DefaultProjectorFn`

This could be easily fixed by explicitly providing a type of projector function (for every createSelector of course):

export function createSelector<State, S1, Result>(
  s1: Selector<State, S1>,
  projector: (s1: S1) => Result
): MemoizedSelector<State, Result, (s1: S1) => Result>;

Maybe this was already discussed? Are there any hidden issues that I'm not aware of?
Otherwise it's strange why this was not escalated yet.

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

[x] Yes
[ ] No

I have all the changes ready to be reviewed - just let me know if the solution here is appropriate.

[timdeschryver]

The change looks good to me!

[alex-okrushko]

So even the usage of DefaultProjectorFn I had to revert within Google's repo to AnyFn, because some of the quirks of the TS. For example, in TS accessing Array by Index always gives you the result with the type and never undefined, which is clearly not what it should be (microsoft/TypeScript#13778)

I tried to demonstrate the problem here

Of course, there is a solution - to give the explicit return type to the projection function, and maybe that's the right solution, but it's hard when this change is introduce later into a huge codebase.

What I what to highlight here is that .projection(...) is used in tests only and if you pass something that is mistyped into it - you'll most likely get an error. Yes, it'll be a runtime error of the failing test and not compile time error, but "fixing" it might create more problems upgrading to a newer version of NgRx. Is it worth it? I'm not sure.

[dzonatan]

I hear you and agree that this is kinda a breaking-change. Even the example-app tests in this repo has a few usages of projector func that would break. But by not having type-safety here we're losing other perks like language-service support - rename and other stuff.. now when refactoring you have to do it in old way with find & replace. As you said - most of the time this will result in a runtime error, but not every - so because of these few times you still have to go through all your usages and fix it manually, otherwise your codebase soon will lose integrity.

Also projector function is not technically limited to tests only. I've seen some cases when it was used in the implementation, although I'm not convinced it is a good practice.

[dev054]

Hey, any chance to revisit it before v11? It's a pain to use projector without type-safety.

[NicBright]

I've run into the same typings problem and wished the .projector was properly typed.

Quoting @alex-okrushko

What I what to highlight here is that .projection(...) is used in tests only and if you pass something that is mistyped into it - you'll most likely get an error. Yes, it'll be a runtime error of the failing test and not compile time error, but "fixing" it might create more problems upgrading to a newer version of NgRx. Is it worth it? I'm not sure.

Yes, fixing this issue is definitely worth it, because NgRx itself is a project written in TS. For my part, this makes me expect that types should be handled properly, even in tests.

Regarding the breaking change dilemma. When you don't want to break existing functions, there's a classic solution: provide the fixed version under a new name, deprecate the old name, wait for a given period and then remove the old version.

You can even do it the other way round (with a little bit of extra effort): rename the current version to maybe createSelectorDeprecated (and provide the fixed version under the current name createSelector). Additionally, provide a script that automatically updates all imports with an alias (I'm thinking of an update helper similar to ng update).

Previously: import { createSelector } from '@ngrx/store'
With alias: import { createSelectorDeprecated as createSelector } from '@ngrx/store'

[david-shortman]

What about introducing a new test method instead that is type safe @alex-okrushko?

proposed tiny change: david-shortman@e7065b1

By introducing a (hopefully someone could come up with a better named) function like testProjector, we could be able to do the following without any need to break and migrate existing code, and it would be strongly typed:

const selectSomeSum = createSelector(a => a, b => b, (a, b) => a + b);
const result = testProjector(selectSomeSum, 1, 2); // 3

this would be enabled by the createSelector updates in #3023 which make inferring the results of the selectors of which a selector is composed easy.

[MaximSagan]

Just created an issue that was closed as a duplicate of this. Let's not sit on this for another two years. If #3023 (which I really hope will be included in v13) already introduces breaking changes in the MemoizedSelector type, it seems like a good opportunity to include this too.

Of course, there is a solution - to give the explicit return type to the projection function, and maybe that's the right solution, but it's hard when this change is introduce later into a huge codebase.

What I what to highlight here is that .projection(...) is used in tests only and if you pass something that is mistyped into it - you'll most likely get an error. Yes, it'll be a runtime error of the failing test and not compile time error, but "fixing" it might create more problems upgrading to a newer version of NgRx. Is it worth it? I'm not sure.

+1 Definitely worth it. I don't even see it being a cost for a huge codebase, even if they've been using .projector() extensively in their unit tests. They're getting something that they lacked before, and something that quite frankly they should be expecting in a Typescript project: type safety.

[david-shortman]

We can make this a gentle transition.

The projector method can have two signatures. One which is strictly typed, and one which is loosely typed.

Then when this change is introduced in some version, it is not breaking, but the loosely typed signature can be marked as deprecated.

Then in the subsequent version, the deprecated signature is removed.

I'd be happy to open a PR with this approach.

[david-shortman]

yolo, went for it: #3223

[david-shortman]

I've authored a PR that demonstrates a soft introduction for strictness that doesn't affect production code or existing tests.

#3360

[david-shortman]

@timdeschryver It looks like v14 is approaching with many new PRs. Is there interest in tackling this in v14? My currently open PR uses an opt-in strategy and suggests a path for future migration to carefully take people toward strict projectors.

[david-shortman]

I'm up for the challenge of creating a type-based eslint rule if that is considered to be the smoothest transition path. Feel free to ping if a direction is chosen.

[aokrushk]

Alright, I'm now in the camp of strongly supporting the proper typing of projector function.

We also ran into this issue and tests didn't catch the mistyped property.

[david-shortman]

Personally, I think the simplest route would somewhat mirror Angular's strictly typed forms.

1. Refactor createSelector to strongly type the selector's projector
2. Migrate existing codebases with a schematic to change calls to use loose typing, like someSelector.projector<any>(...)
3. Developers can remove the generic argument any and the projector will be strongly typed

[timdeschryver]

Thanks for the idea @david-shortman, I like it!
We're verifying the last pointers, but I think we should try to land this in v15.

[timdeschryver]

@david-shortman and others, we discussed this and we want to go forward with these changes.
If possible, these would be included in the v15 release.

I created two issues for this:

• Typed projectors: Selectors: typed projector methods #3571
• Migration: Migration: typed projector methods #3572

Thanks for the input!