Run selectors twice in development to warn for memoization failures

[markerikson]

This one has been asked about for a while, particularly by @theKashey :

• Reselect v5 Roadmap Discussion: Goals and API Design #491 (comment)
• https://twitter.com/keeslinp/status/1656342003268071424

The idea is that users sometimes end up writing badly-written selectors that accidentally recalculate the results every time. This is primarily due to badly-written input functions, such as:

createSelector(
  state => ({ foo: state.foo, bar: state.bar }), 
  ({ foo, bar }) => ({result: foo + bar})
)

Since the input function always returns a new object, the output function will always run and return a new reference.

Probably the simplest thing we can do here would be to double-run the input functions twice and verify that the results are shallow equal.

[theKashey]

Basically following the React StrictMode logic - run everything twice to reveal issues, 😞 even if it might create some issues.
Would you consider making this feature an opt-in like StrictMode in React?

Might be it's better to first implement it in mapStateToProps or useSelector - something you have more control of.

[markerikson]

We've got a proof-of-concept PR up that does this.

Biggest questions:

• Should we do the check every time the selector runs? Or just once per selector?
• Should the double-check use just shallow equality, or should it use the same memoizer + options that the user provided?
• Should this be on by default, or opt-in?

[josh-degraw]

I think this is a great idea, but I think it should absolutely be opt in like react's strict mode checks. I could see it causing a lot of unnecessary bug reports from people that didn't know the feature was coming,

[EskiMojo14]

I don't see what bug reports would be raised? The change means that a warning is logged if (and only if) any of the input selectors return a different reference when called with the same arguments.

Input selectors should never have side effects, so I don't see it as equivalent to StrictMode. Invoking input selectors twice should only at worst impact performance in non-production environments.

[josh-degraw]

Fair enough. I guess I'm just having flashbacks the strict mode update where tons of libraries were getting bug reports that were in-app problems but they didn't realize and library maintainers had to deal with all that haha. But you're right this is a different problem set so as long as the messaging is clear and it's optional somehow it should be fine either way

[EskiMojo14]

currently i've enabled opting out via the CreateSelectorOptions:

const selector = createSelector([...inputs], output, { inputStabilityCheck: false });

this could get inconvenient if you wanted to disable it for an entire set of selectors, so it may be worth seeing if createSelectorCreator's signature can be revised in a way that allows for passing a config object.

edit:

I've removed the per selector config in favour of a lib level flag:

import { setInputStabilityCheckEnabled} from 'reselect'

setInputStabilityCheckEnabled(false)

// now disabled

setInputStabilityCheckEnabled(true)

// enabled again

i think the chances of a user wanting the check for some selectors and not for others is essentially nil.

[theKashey]

Let's first split the use cases:

• pure createSelector
  • maybe a new option (like in the comment above)
• testing of different sorts
  • maybe a configuration command (like in the comment above)
  • may be a testing helper - expect(() => selector).toBeStable() || expect(isStableSelector(selector, args)).toBeTrue()
• testing at the application level
  • a global configuration (for redux or reselect?)
  • a StableSelectors component configuring behaviour

Also, the end user might want to configure a warning or an error - it depends on the end-user and the platform should delegate such a decision to them.
Also, the end user might need some advice on fixing a broken selector. For example, the one at the thread start cannot be fixed, unless you change it or use proxy-based solutions.
Also, there should always be an escape hatch to "accept" some cases to be incorrectly implemented, just because you don't need everything to be perfectly pure and there are cases when accepting a defect is not bad.

[EskiMojo14]

I've added back the per selector config, and added a "once" option which is now the default - meaning the check is only done on the first run of the selector. This seems like a nice middle ground, which will catch most badly written input functions.

just because you don't need everything to be perfectly pure and there are cases when accepting a defect is not bad.

@theKashey Could you give an example? I can't think of a situation where an input selector returning a new reference every time the selector is run would be a good thing, as it would completely blow out the cache every time.

Additionally, I can't see a situation where having side effects in an input selector is at all advisable. Selectors should be entirely pure.

[mitroc]

The idea is great, but please let it be opt-in. I know that using poorly optimized projects as examples is not ideal, but they do exist, and in such cases immutableCheck and serializableCheck need to be disabled, otherwise coding becomes impossible. So I imagine (of course, I could misunderstood the entire idea) that the default behavior of running all selectors twice could significantly worsen the developer experience.

[EskiMojo14]

@mitroc I've updated the README in the PR's branch - does that make the idea clearer?

I don't really understand the argument that projects using bad practices mean that checks to assist developers in following better practices should be opt in. Like I've said, it's simple to disable the check globally, or per selector.

The default behaviour only runs the check the first time a selector is used, so I imagine this shouldn't hinder performance significantly past first load of a feature.

[guillaumebrunerie]

While I do think it is a very good idea to run selectors twice in development to detect improperly memoized selectors, I feel like (at least for React) that it should be done in useSelector directly instead, as doing it in createSelector will not detect the common case where someone is passing to useSelector a function that always creates new reference (without using createSelector at all).

[EskiMojo14]

That could also be done, feel free to raise an issue in the react-redux repo suggesting it.