Idea: wrapping createSelector

[msutkowski]

Just wanted to get a log of some ideas here in regards to: immerjs/immer#561 and the original issue it references: #452

@phryneas and I were speaking earlier and brainstormed a few ideas for how this issue could be addressed. Currently, once we bump to v1.4 (or whichever has immer 7.0.1), a user could solve this themselves by doing something like selectorA(current(state)) inside of a reducer. Being that we use immer everywhere, we could provide a wrapped version of createSelector and/or entity adapter selectors that does this automatically for a user.

Basically, something that would achieve this:
const wrappedEntitySelector = (value) => isDraft(value) ? entitySelector(current(value)) : entitySelector(value)

@phryneas I might've forgotten part of that convo, so please correct me if I'm missing something

[markerikson]

I'm not convinced there's a serious need to support the use cases of using selectors inside of a reducer. I get that there's some benefit, but given that you're operating on a more limited section of the state, it doesn't seem as useful.

[phryneas]

Yeah, we had that though, too.
But on the other hand:

• we are shipping the entityadapter selectors
• those are a great match to write readable code in reducers, so people will do it
• doing so more than once will introduce bugs like

createReducer(..., {
  case1(state, action) {
    const itemBefore = selectById(state, action.payload.id);
    adapter.updateOneMutably(state, action.payload);
    const itemAfter = selectById(state, action.payload.id);

    // one would assume that itemAfter is now different from itemBefore, but since state is still the same object, itemAfter === itemBefore, even if the state would be different
  }
});

• preventing that bug is realtively simple and does not cost performance (except for one isDraft check, which should be pretty cheap) outside of situations where, until now, it would be a bug.

=> I would suggest definitely doing this for the entityAdapter selectors.
And from there, I would either transparently wrap createSelector (just modifying the equality check would not really work out), or export a second variant createDraftableSelector or something.

The point is: while from an reselect standpoint, shipping a wrapped createSelector does not really make sense, from an RTK standpoint shipping an "immer-compatible" variant with almost no overhead (other than in situations where it would otherwise have been a bug) seems like the right thing to do.

[markerikson]

My main worry is that we end up in a situation where we're having to spend a bunch of effort dealing with the wrapping layer for various situations, or people get used to be able to call a selector in one place and then it behaves differently somewhere else.

[phryneas]

I just noticed that Matt pasted an early discussion example - that might not 100% reflect the basic idea.

The idea would be to export

import {createSelector as createReselectSelector} from 'reselect' 
export const createSelector: typeof createReselectSelector = (...args) => {
  const selector = createReselectSelector(...args);
  return (value, ...rest) => selector(isDraft(value) ? current(value) : value, ...rest)
}

So this would affect all our selectors and every selector created by users using our version of createSelector. No different behavior for different usage.

Also, this is primarily a hack around the equality check to somehow handle the "same proxy for different contents" problem, so it should not really be "different behavior" from a user's perspective.

[nathggns]

FWIW, I was just bitten by the bug of reselect selectors returning stale data when ran on Immer proxies. Personally, I think the whole point of RTK is to be opinionated – so it should either prevent/warn/heavily discourage using reselect selectors on immer proxies, or it should support it. This silent bug is a really tricky one to catch and I just lost more than an hour trying to track it down. I'm a huge fan of RTK, and this definitely seems like something RTK should have an answer to.

It might be worth considering whether its better for performance to use current in the wraper or if RTK could modify the equality check to always return false when detecting an Immer proxy.

[phryneas]

@markerikson if I remember correctly, we recently had the same thing come up in another discussion, so people are definitely trying to use these selectors in reducers.

So as I'm preparing v1.5, I want to bring this up again:

Could we reconsider to either wrap createSelector in general, or at least to it for the createEntityAdapter selectors?

[markerikson]

I do not want to "transparently wrap createSelector". That introduces a delta in behavior between happening to import it from RTK and from Reselect.

I could possibly live with doing it for the generated entity selectors, because those are done internally.

On the other hand, the globalized entity selectors aren't usable in a slice anyway because they expect the entire state.

[phryneas]

I do not want to "transparently wrap createSelector". That introduces a delta in behavior between happening to import it from RTK and from Reselect.

Fair enough.

I could possibly live with doing it for the generated entity selectors, because those are done internally.

I'll open a PR for that.

On the other hand, the globalized entity selectors aren't usable in a slice anyway because they expect the entire state.

Well, people seem to create "localized" versions of those 🙈
It doesn't really hurt anyone and might save some people their sanity :)

[markerikson]

I think this got resolved by the "draftable selector" thingamajig:

https://redux-toolkit.js.org/api/createSelector#createdraftsafeselector