Platform: 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'