CreateSelectorFactory returns entire state when provided an empty array of selectors

[dezmon-fernandez]

Explanation

Providing createSelectorFactory n selectors yields inconsistent results between v6 and v7.

In version 6 if createSelectorFactory was provided an array of empty selectors, the projectorFunction would be supplied an empty array.

In version 7 the projectorFunction is supplied an array of the entire state.

Minimal reproduction of the bug/regression with instructions:

Refer to console in each stackblitz

v6
https://stackblitz.com/edit/typescript-createselectorfactory-v6

v7
https://stackblitz.com/edit/typescript-createselectorfactory-v7

Expected behavior:

Retain functionality and return an empty array if no selectors were provided.
Is there a better way to get the result of a dynamic number of selectors?

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

ngrx v6 -> v7

Use case:

I have x members at any time which i need to query information on and display statuses, which is why i need dynamic selectors.

[timdeschryver]

I think that the current implementation is correct, but also that the previous implementation was correct. The reasoning behind it is the following:

• When you don't have any selectors/getters defined, it should invoke the projector function. This causes the behavior in your v7 example to return the state.
• A createSelector without a selector/getter wasn't possible in v6. The only reason why it returns an empty array in your v6 example is because you're using the spread operator in your selector. Change the implementation to (statuses) => statuses and it will log out undefined, because there are no selectors/getter to return anything.

The change that causes this:
The selector with props feature, introduced a createSelector function without the need to define a selector/getter. With this change the following lines
https://github.com/ngrx/platform/blob/master/modules/store/src/selector.ts#L554 were added to directly call the projector in this case. In your case, this also means that we directly call the projector function and that the state will be passed to the projector function (instead of undefined because there are no selectors).

These changes maked the following possible:

const getTodosById = createSelector(
  (state: TodoAppSchema, id: number) => state.todos.find(p => p.id === id)
);

Instead of the need to create a selector/getter:

const getTodosById = createSelector(
  (state: TodoAppSchema) => state,
  (todos: Todo[], id: number) => todos.find(p => p.id === id)
);

To end, I think you made a selector with only a projection function which isn't possible if you would try this to do this in a "typed" way e.g. createSelector(x => x) gives an error in v6. Because there are no selectors, the projector function retrieves an undefined, and you mapped this undefined to an empty array. In my opinion, this is bending the behavior to your needs (which is not necessarily a bad thing).

In order to "fix" this:

a) we can remove the props selector with only a projection function, thus also the extra check
b) we can only invoke the projection function if there are indeed props,
which is in fact not even possible in a typed way, for example

const selector = createSelector(x => x) 
xxx(47) // gives the error 'Expected 2 arguments, but got 1.'
xxx(47, props) // is OK

// proposed fix
if (selectors.length === 0 && props !== undefined) {
   return projector.apply(null, [state, props]);
}

Personally, I think we should leave it as it is, but I'm more than happy to discuss it further.

Meanwhile, to get your existing code to work you could use:

const selectStatuses = (ids: any[]) => {
   const selectors = ids.map((id) => getStatus(id) );
   return createSelectorFactory(defaultMemoize)(
        ...(selectors.length ? selectors : [() => []]),
       (statuses) => {
        return statuses;
       }
   );
}

[dezmon-fernandez]

Thank you for your response and solution, the guard works for what I need.

Interesting, makes sense. It begs the question should the user have to guard against 'empty selectors' or should the projector function even be called if there are no props as you proposed above? I'm all for type safety; however, I feel at this point it is almost tribal knowledge that the projector function will be called with different types if the selectors provided are empty.

ie

const selectors = [];
createSelectorFactory( 
  ...selectors,
  (...selectorResults) => selectorResults  // [{...state}, null]

vs 

const selecotrs = [getAStatus, getBStatus];
createSelectorFactory(
   ...selectors, 
  (...selectorResults) => selectorResults  // [aStatus, bStatus]

One other note is with the workaround I need to provide the initial state for the empty selector case.
Say I have a selector that queries if any user is online.

const selectIsAnyUserOnline = (userIDs) => {
  const selectors = userIDs.map( uid => selectIsUserOnline(uid) );
  createSelectorFactory(
    ...(selectors .length ? selectors : [() => false]), 
    (...usersOnline) => usersOnline.some( isOnline => isOnline) 

}

I can't think of this being detrimental, but just something to think about when using this approach.