Platform: Enforce or drop store type parameter

[rehmsen]

The type parameter T in Store<T> is not actually enforced when piping a selector onto it, so you can basically apply a MemoizedSelector<U,V> even when T does not extend U. At least part of the problem seems to be the object/any types of createFeatureSelector here:
https://github.com/ngrx/platform/blob/55a0488c7e456d74affa2354a12cc20db12e5f38/modules/store/src/selector.ts#L272

I think it would be better to either actually enforce T extends U, or drop the type parameter from Store to acknowledge that it's not strictly typed.

Describe any alternatives/workarounds you're currently using

As this is not enforced, we currently always use Store<{}>, and just rely on the programmer to use the appropriate selector that can deal with our application-wide store. This works, but seems a bit like a hack.

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

[ ] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No
[x] Maybe :) In general, yes, but I have not yet tried to fix this and am not sure I can. I can give it a shot.

[sandangel]

I think this is because your feature state doesn't reflect the whole store interface (and we definitely don't want to), so we can only use object type there (memoized selector). The type parameter still provide typesafe in case you use string selector.

[timdeschryver]

@brandonroberts I think @sandangel provided the right answer here.
But I'm hesitant to close it, before hearing your opinion on this.

[sandangel]

I think if users are using {} for type parameter like this, we could provide a default type for the type parameter. How about using Store<T = object> ?

[rehmsen]

I played with this.

It seems that you can use a type parameter instead of object. I don't think it matters if T is the whole store interface - as long as the type of the state you put in matches the interface of your type parameter (so the state can have features a,b and c, even if the type T only has a - the usual TS structural typing).
My playground shows that this typing will prevent you from mistakenly feed in a state that does not match the type parameter T.

Two things that I still do not like that much, but am not sure how to fix:
1) It still compiles even when you pass in a T and featureName that are not compatible.
2) The any is a little ugly, but it does not really seem to hurt because we explicitly set the return type.

Does this make sense, or am I missing something?

[rehmsen]

I think I also found a solution for 1.:

// Simplified selector interface, Memoization should not matter
interface Selector<T, V> {
    (state: T): V;
}


function createFeatureSelector<T, V>(featureName: keyof T): Selector<T, V> {
    return (t: any) => t[featureName]; 
}

interface FooFeatureState {
    bar: string;
}

interface StateWithFoo {
    ['foo']: FooFeatureState;
}

// Compiles as it should
const f = createFeatureSelector<StateWithFoo, FooFeatureState>('foo');
// Does not compile because foz is not keyof StateWithFoo
const g = createFeatureSelector<StateWithFoo, FooFeatureState>('foz');

const stateWithoutFoo = {
    baz: false,
};

const stateWithFooAndOthers = {
    baz: false,
    foo: {bar: 'hello'}
};

// Compiles because there is foo. The other features do not hurt.
f(stateWithFooAndOthers)

// Does not compile because it's missing foo
f(stateWithoutFoo);

This is still not perfect because the type system does not enforce that T's field with name featureName is of type V, but it seems better than what we currently have. Let me know if I am overlooking something or if I should prepare a pull request.

[timdeschryver]

@rehmsen I came up with a similar idea a couple of days go, but I think this can't be done.
Ref: #1137

[rehmsen]

Thanks for sharing - can you elaborate what the error with combineReducers was?

[timdeschryver]

@rehmsen You can ignore what I said, I was looking at the wrong places (got a bit too excited)... 😅
I think this solution would prevent some errors that you otherwise would have at runtime.
I'm in favor of this, but I don't know if @brandonroberts would agree - so let's wait what he has to say before you submit a PR.

I was trying to determine V automatically, and there is where it went wrong for me.
So you would only have to do:

createFeatureSelector<StateWithFoo>('foo'); // this would automatically know that `FooFeatureState` is the return value

But I don't know if this would be possible in TypeScript.

[brandonroberts]

AFAIK we can't determine that automatically, since we're not providing the overall state interface. We can add support for this without breaking the signature with an overload.

export function createFeatureSelector<T>(
  featureName: string
): MemoizedSelector<object, T>;
export function createFeatureSelector<T, V>(
  featureName: keyof T
): MemoizedSelector<T, V>;
export function createFeatureSelector(
  featureName: any
): MemoizedSelector<any, any> {
  return createSelector(
    (state: any) => state[featureName],
    (featureState: any) => featureState
  );
}