Parametrised selectors memoization

[irmantaszenkus]

Hello, according to docs and source code, all selectors are created with createSelector from reselect. reselect does not provide a way to handle parametrised selectors and recommends taking approach like such

const createNameSelector = () => createSelector(selectById, selectName)

And then in react-redux version we would create selector instance for each component that needs it

connect(() => {
  const selectName = createNameSelector();
  return (state, { id }) => ({
    name: selectName(state, id)
  })
})

When using redux-toolkit and creating globalized selectors like this

const globalizedSelectors = personsAdapter.getSelectors(state => state.persons);

globalizedSelectors becomes this single instance and if I were to use it like this

connect(
  (state, {id}) => ({
    name: globalizedSelectors.selectById(state, id)
  })
)

Selector memoization would become pointless, as each component will pass different id and overwrite cached version.

I could however do something like this

const createNameSelector = () => createSelector(
  globalizedSelectors.getSelectors(state => state.persons).selectById,
  selectName
);

And have instances created each time. But then in my opinion it would be efficient to access non-memoized selectors from redux-toolkit and use them like this

const createNameSelector = () => createSelector(
  globalizedSelectors.selectors.selectById,
  selectName
)

[phryneas]

Hm, that is actually a real problem. selectById is defined as

createDraftSafeSelector(
        selectGlobalizedEntities,
        selectId,
        selectById
      )

and while selectId and selectById have irrelevant execution times, actually selectGlobalizedEntities should not re-run on every argument change.

Coincidentally, that should already get solved by accident in #990 - can you take a look and verify that?

[markerikson]

It's important to realize that for selectById, memoization doesn't matter at all - it's a straight lookup, not actual derived data. Ditto for selectGlobalizedEntities.

[phryneas]

@markerikson not 100% agreeing. Assuming you have 100 components and many entities, I would not want selectGlobalizedEntities to re-run, because it is doing a map over the whole array. I would want that to be memoized.
selectById would be irrelevant though.

[markerikson]

selectGlobalizedEntities should be memoizing correctly, though:

    const selectGlobalizedEntities = createDraftSafeSelector(
      selectState,
      selectEntities
    )

It's only going to re-run the output selector once the entities state changes, and even then it's a straight lookup: const selectEntities = (state: EntityState<T>) => state.entities. So, nothing to memoize anyway.

[phryneas]

Ah right, it's prefaced with selectState, I somehow forgot about that part.

[irmantaszenkus]

It seems that none of 5 memoized selector actually benefit from reselect. selectEntities, selectAll, selectIds do work correctly but they are straight accessors and have no derived state computation. selectById is also just an accessor too and does not need memoization, but instead it gets memoized and then cache is cleaned up each time new id is passed.
If you support my arguments, I would like to contribute and remove memoization

[phryneas]

selectAll is not a straight accessor and does benefit from memoization - quite a lot over only calling it twice.

In general: it doesn't hurt having it in there.
No application should ever hit a performance level where having reselect here or not would make any perceivable difference.

[irmantaszenkus]

But it does hurt very very slightly for selectors like selectById, by executing code, that memoizes return value. And at the same time it does not provide any benefit.
Would it make sense to return memoized selectors for all of them except parametrised selectById?

[markerikson]

We're talking a couple comparisons and a map() call inside of createSelector. This is not meaningful perf-wise, at all.

Realistically, selectAll is the only one that does any derivation, so it's the only one that needs to be written using createSelector. But, I'm not seeing enough reason to really make any changes here atm.