Type inference for createSelector is broken since changing to variadic tuple types

[jboeijenga]

Minimal reproduction of the bug/regression with instructions:

const state: {first : string} = { first: 'state' };

const getData = <T>(d: T): T => d;
const returnState = () => state;

const selector = createSelector(returnState, getData);
expect(selector(state).first).toEqual('state');

As of v13 above code will give a compile error saying: Object is of type 'unknown'.

Expected behavior:

Expected that createSelector would be able to infer the return type for selector correctly to {first : string}

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

NgRx 13

[david-shortman]

Contextual inference for the projector's return value based on its arguments fails when it's variadic. Not sure of the solution yet.

Here's a reproduction that shows the inference works when explicitly declaring a creator function which only takes in "one slice" of state vs one that takes in a variadic number of slices: playground

type Selector<T, V> = (state: T) => V;

declare function contextualInferenceOneSlice<T, U>(...args: [getSlice: () => T, projector: (c: T) => U]): Selector<unknown, U>;
declare function contextualInferenceVariadicSlices<Slices extends unknown[], U>(...args: [...getSlices: { [i in keyof Slices]: Selector<unknown, Slices[i]> }, projector: (...args: Slices) => U]): Selector<unknown, U>;

const state: { first: string } = {first: 'state'};

const getData = <T>(d: T) => d;
const returnState = () => state;

const oneSlice = contextualInferenceOneSlice(returnState, getData);
const test1 = oneSlice(state).first;

const variadicSlices = contextualInferenceVariadicSlices(returnState, getData);
const test2 = variadicSlices(state).first; // error

[david-shortman]

Note that the inference works fine when providing the function plainly without generics:

const goodSelector = createSelector(returnState, d => d);
const test1 = goodSelector(state).first; // works

const badSelector = createSelector(returnState, <T>(d: T) => d);
const test2 = badSelector(state).first; // error

[david-shortman]

Finding the mechanism by which T is unable to be inferred in getData (hence becoming unknown) will resolve this issue.

[david-shortman]

Shooting my shot on SO 😅- https://stackoverflow.com/questions/70439616/why-does-contextual-inference-fail-for-the-return-type-of-previously-declared-fu

[david-shortman]

Filed an issue in TS: microsoft/TypeScript#47226

Not sure if the issue by design. But we may need to implement overload signatures again to patch this regression, as recommended in https://stackoverflow.com/a/70469205/11087018

[c-goldschmidt]

For anyone running into this issue: 13.0.0-beta.0 works with angular 13 and does not contain the changes to the interfaces.

[AmebaBrain]

For anyone running into this issue: 13.0.0-beta.0 works with angular 13 and does not contain the changes to the interfaces.

This is actually a madness of a new type definition. With a mix of a new createFeature approach I burnt 3 hours before got into this issue opened.

Offtopic: With addition of new createFeature function with single reducer definition this release is kind of strange. When you have nested objects in your feature object you still have to pack everything into a single reducer function. Just fo the sake of getting auto-generated feature attributes selector names? Strange release...

[timdeschryver]

We're sorry to hear that @AmebaBrain - how can we improve this? If it's related to createFeature, please open a new discussion or issue.

To be clear, the createFeature function has nothing to do with the introduction of variadic tuple types for selectors.

[mattlewis92]

Hey there, we just ran into this issue when upgrading our application to v13, given that the problem doesn't seem possible to solve with the version of typescript angular v13 is currently on, I guess there's no other option but to revert the variadic tuple types? Happy to assist with this, would it be possible to land it in a 13.x version of ngrx or would it need to wait for v14?

[timdeschryver]

@david-shortman we could provide both options, right? Keep the tuple for most of the selectors (and allow X amount of child selectors), but re-add the removed selectors as a fallback option?

[david-shortman]

Yeah. I was hoping to see more activity on microsoft/TypeScript#47226, but it's only been marked as "Needs Investigation".

I'll author a PR.

[david-shortman]

However, I'm not sure transitioning back is worth it. Here's the two broken scenarios I see, and why they have a small impact:

1. The example in this issue only applies to generic projectors with no constraints declared for reusability outside of an invocation of createSelector. Most selectors are declared with unique projectors written directly as an argument to createSelector. If a function is declared separately for reuse across selectors, it seems more likely that they would have a constraint:

// Say we have a state like so
const state: { a: string, b: number } = { a: 'a', b: 2 };

// We could have a reusable projector like this, but it's kinda useless
const identity = <T>(t: T) => t;
// This type of reusable projector seems more useful, and has a constraint on the generic argument
const getA = <T extends { a: string }>(t: T) => t.a;
const getState = () => state;

const oneSlice = createSelector(getState, identity);
const test1: string = oneSlice(state).a; // this has an error because inference fails and oneSlice is determined to operate on unknown instead of the state

const variadicSlices = createSelector(getState, getA);
const test2 = variadicSlices(state); // this works just fine, test2 is inferred to be a string

2. Some folks have had issues with the overload for createSelector being inferred to be one that uses SelectorWithProps. This issue will go away when Remove SelectorWithProps #3035 is implemented, which I'd think would happen in the next couple of versions.

[mattlewis92]

However, I'm not sure transitioning back is worth it. Here's the two broken scenarios I see, and why they have a small impact:

This bug actually causes 528 type errors on our application. Mostly because we have a lot of legacy selectors that followed the very old ngrx style guide that recommended separating createSelector from the projector function (so that unit testing was possible, as it was before we had .projector).

We can and will get rid of these usages but given that this breakage wasn't intended, documented or given a proper deprecation lifecycle I think the right course of action would be to transition back at least for a few major versions to give people time to migrate away.

[david-shortman]

Mostly because we have a lot of legacy selectors that followed the very old ngrx style guide that recommended separating createSelector from the projector function (so that unit testing was possible, as it was before we had .projector).

I believe those errors fall into bucket 2. If we get rid of selectors with props, then those selectors will not be incorrectly inferred as selector with props.

Still, since that's not on the horizon that I'm aware of, it sounds like there's enough motivation to put back the explicit overloads.

[mattlewis92]

I believe those errors fall into bucket 2. If we get rid of selectors with props, then those selectors will not be incorrectly inferred as selector with props.

Ahh I see, that makes sense now and I see what's happening

Still, since that's not on the horizon that I'm aware of, it sounds like there's enough motivation to put back the explicit overloads.

Awesome, let me know if there's anything I can do to help! 🙌

[timdeschryver]

@david-shortman we did just discuss this, and if you want you can re-add the removed selector types.
This way we don't break existing codebases, and we still provide the option to have more selectors than before.
We can remove the typed selectors in a next release as a breaking change.

[brandonroberts]

Second what Tim said. We'll cut a release after that PR lands