Platform: Using non-parametrized selector inside parametrized may result in multiple calls

[Akxe]

Minimal reproduction of the bug/regression with instructions:

interface Store {
  elements: unknown[];
}

export const offerState = createFeatureSelector<Store>('store');

const listAsSet = createSelector(
    state => state.list,
    carriers => new Set(carriers),
);

const elementSelectorFactory = () => createSelector(
    listAsSet,
    (set, { element }) => set.has(element),
);

Expected behavior:

I would expect it to behave like the fix below.

interface Store {
  elements: unknown[];
}

export const offerState = createFeatureSelector<Store>('store');

const listAsSet = createSelector(
    state => state.list,
    carriers => new Set(carriers),
);

const elementSelectorFactory = () => createSelector(
    state => listAsSet(state),  // This will fix it, but this is nor documented, nor user friendly...
    (set, { element }) => set.has(element),
);

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

NgRx: 10.1.2

Other information:

In my opinion, the selector should be aware if it uses the props in its projection function. Only if it uses props they should get passed to it. Function.length might help here.

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

[ ] Yes (Assistance is provided if you need help submitting a pull request)
[x] Once agreed upon solution, it might be finicky...
[ ] No

[Akxe]

I can see 3 possible solutions:
1) Remove params from createSelector, instead create a createSelectorWithParams function, that would require params accept. This could also be used to mitigate the factory that needs to be created around the selector called with multiple different props.
2) Relly on projectionFn.lenght vs arguments.lenght - 1, to decide, if selector uses the props.
3) projection and projectionWithProps as the last parameter of createSelector. They would give the selector a hint on wherever or not pass parameters to the given selector.

[timdeschryver]

Thanks for creating this issue @Akxe , the issue here is that the props are passed down to all selectors.
Because of this listAsSet will be invoked multiple times, once for each time a listAsSet selector is initialized.

If this is not the behavior that you want, I would suggest passing the props to the selector via a factory method:

const elementSelectorFactory = (element) => createSelector(
    listAsSet,
    (set) => set.has(element),
);

[Akxe]

@timdeschryver I know why it happens, i debugged the code quite a bit. I would suggest filling docs on this matter.

Why is this factory a standard? There is whole mechanism for props, but you can never use them, because of this problem...

[markostanimirovic]

+1 to think about removing createSelector overloads with props, because it can be replaced with factory function.
Also, without props, createSelector arguments can be switched to variadic tuples to accept any number of input selectors :)

[Akxe]

@markostanimirovic To allow any number of input selectors I would suggest accepting an object as the first argument. Just like combineLatest does from v7. The key benefit of this switch/addition is, that it is less error-prone.

createSelector({
  a: aSelector,
  b: bSelector,
}, ({a, b}) => `${a} ${b}`);

It is almost the same, but the object notation is very well supported in TS, unlike variadic tuples, it is there only since 4.x.x. Thought it might not be a problem for angular since your support is tied to the Angular version that supports TS 4.0.0

---

How about removing props from createSelector, but having them in createSelectorWithProps?

[timdeschryver]

I agree that createSelector (with props) needs some extra work.
It's a bit late to add it to a v11 release, but we should discuss this for a future release.

What we can do now, is polishing up the docs around selectors with props (e.g. mention that used selectors will be executed and document that you can pass in arguments via the factory method to prevent the described issue).

[markostanimirovic]

@markostanimirovic To allow any number of input selectors I would suggest accepting an object as the first argument. Just like combineLatest does from v7.

createSelector({
  a: aSelector,
  b: bSelector,
}, ({a, b}) => `${a} ${b}`);

@Akxe
Current signature of createSelector looks better to me, especially when you have only one selector:

• Sequence (current):
  createSelector(selectA, a => a + 1);

• Object:
  createSelector({ a: selectA }, ({ a }) => a + 1);

The key benefit of this switch/addition is, that it is less error-prone.

Can you give me an example where current createSelector signature is error prone?

It is almost the same, but the object notation is very well supported in TS, unlike variadic tuples, it is there only since 4.x.x. Thought it might not be a problem for angular since your support is tied to the Angular version that supports TS 4.0.0

Yes, NgRx now supports TS 4 and strong typing for any number of input selectors can be done with variadic tuples 🙂

[Akxe]

If you ever combine 5+ operators, it gets messy if you want to add or remove one. Even more, if you want to organize them.

---

export const createURLSelector = createSelector(
    privileges,
    currentGroup,
    groups,
    regions,
    districts,
    (
        privileges,
        currentGroup,
        groups,
        regions,
        districts,
    ) => {
        return {
            canSeePrices: privileges.canSeePrices,
            canEditWanted: privileges.canEditWanted,
            canAssignMotives: privileges.canAssignMotives,
            canCreateMotives: privileges.canCreateMotives,
            currentGroup,
            groups,
            regions,
            districts,
        };
    },
);

Removing a selector from the code above is a bit more painful. The best would be to support both notations, just like combineLatest (will) do.

---

I usually like to have similar keys together (eg.: currentCarrier, currentPeriod, currentGroup).