Platform: Store: Types are not inferred in projector function when using props

Created on 1 Nov 2019  路  9Comments  路  Source: ngrx/platform

Minimal reproduction of the bug/regression with instructions:

GitHub Repo: https://github.com/brandonroberts/ngrx-props-bug

  • Clone repo
  • Run yarn
  • Go to the src/app/state/users/users.selectors.ts file
  • Note the selectUserWithoutProps types are inferred in the projector
  • Note the selectUserWithProps types are not inferred in the projector

Expected behavior:

The types still get inferred when using selectors with props.

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

NgRx 8.4.0

Other information:

I would be willing to submit a PR to fix this issue

[ ] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

Store bug community watch

Most helpful comment

Hey @SerkanSipahi, @brandonroberts,
Besides the issue described here there's another issue that I believed is contained in the same part of the code.

When combining parameterized selectors it doesn't take all the props into account as can be seen in this example:
image

The TS error says that props from the first selector is incompatible with the second one. According to the used type that makes sense:

export declare function createSelector<State, Props, S1, Result>(s1: SelectorWithProps<State, Props, S1>, projector: (s1: S1, props: Props) => Result): MemoizedSelectorWithProps<State, Props, Result>;

The generic Props is used twice, so it requires the props objects to be the same for both selectors which is of course not the case/expected (the same case occurs in the docs as well and it fails for the same reason when fully typed).

I want to propose the following fix:

export declare function createSelector<State, S1Props, S1, Props, Result>(s1: SelectorWithProps<State, S1Props, S1>, projector: (s1: S1, props: Props) => Result): MemoizedSelectorWithProps<State, S1Props & Props, Result>;

In this case the props from all selectors are merged so the selectors are defined without problem. The second benefit is that it is typed when using the selector as it displays the correct parameters:
image

I was hoping this issue can be taken along with the main issue as it probably can be fixed in one go, right?

All 9 comments

@brandonroberts @timdeschryver Can i take this Issue?

@SerkanSipahi yep, just request Tim or Alex for review when you're ready

Hey @SerkanSipahi, @brandonroberts,
Besides the issue described here there's another issue that I believed is contained in the same part of the code.

When combining parameterized selectors it doesn't take all the props into account as can be seen in this example:
image

The TS error says that props from the first selector is incompatible with the second one. According to the used type that makes sense:

export declare function createSelector<State, Props, S1, Result>(s1: SelectorWithProps<State, Props, S1>, projector: (s1: S1, props: Props) => Result): MemoizedSelectorWithProps<State, Props, Result>;

The generic Props is used twice, so it requires the props objects to be the same for both selectors which is of course not the case/expected (the same case occurs in the docs as well and it fails for the same reason when fully typed).

I want to propose the following fix:

export declare function createSelector<State, S1Props, S1, Props, Result>(s1: SelectorWithProps<State, S1Props, S1>, projector: (s1: S1, props: Props) => Result): MemoizedSelectorWithProps<State, S1Props & Props, Result>;

In this case the props from all selectors are merged so the selectors are defined without problem. The second benefit is that it is typed when using the selector as it displays the correct parameters:
image

I was hoping this issue can be taken along with the main issue as it probably can be fixed in one go, right?

I'm facing the same problem

I tried figuring out the original issue but to no avail. This is such a weird problem because every time I try to isolate the issue, I get stuck.
I was hoping someone could give some hints in order to fix this issue.

I'm sure I don't understand some of the internal working on NgRx, so hopefully this question makes sense.
But I noticed it a bit hard to work with both MemoizedSelector/Selector and MemoizedSelectorWithProps/SelectorWithProps, shouldn't they simple be one thing and making props optional? Because I don't see any way of reconciling them in a nice fashion.

I can imagine this was done because props was a feature later added to NgRx and was integrated into the existing core but on a side-track.

Would it make sense to try and combine them @timdeschryver @brandonroberts?

I'm facing the same problem too.

@harm-less I think they should remain separate

I pulled the user example into this branch, and saw the type inference working correctly: https://github.com/ngrx/platform/pull/2905

Was this page helpful?
0 / 5 - 0 ratings