Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potentially misleading advice in "Balanced Selector Usage" #4783

Open
jkillian opened this issue Mar 17, 2025 · 6 comments · May be fixed by #4784
Open

Potentially misleading advice in "Balanced Selector Usage" #4783

jkillian opened this issue Mar 17, 2025 · 6 comments · May be fixed by #4784

Comments

@jkillian
Copy link

jkillian commented Mar 17, 2025

What docs page needs to be fixed?

Link

  • Section: Balance Selector Usage
  • Page: Deriving Data with Selectors

What is the problem?

Quoting from the docs:

Similarly, don't make every single selector memoized!. Memoization is only needed if you are truly deriving results, and if the derived results would likely create new references every time. A selector function that does a direct lookup and return of a value should be a plain function, not memoized.

// ❌ DO NOT memoize: deriving data, but will return a consistent result
const selectItemsTotal = state => {
  return state.items.reduce((result, item) => {
    return result + item.total
  }, 0)
}
const selectAllCompleted = state => state.todos.every(todo => todo.completed)

What should be changed to fix the problem?

I'd be curious here if opinions differ, but I believe the advice above is misleading. Specifically, I think there are plenty of good times you want reselect-like memoization even if your selector returns a scalar value.

One big benefit of memoization is that the actual selector computation itself doesn't have to rerun in every component using the selector on every redux action if the inputs haven't changed.

Let's say for example you have 100 <Item /> components currently rendered on screen and 1000 items in the redux store. Each <Item /> does something like useSelector(selectItemsTotal). Without memoization, on every dispatched Redux action you'll end up with 100,000 additions taking place. With memoization, you'll have 1000 additions and 100 equality comparision for memoized selectors which will then opt-out of computation.

As a strawman, I'd potentially change the docs to something like the following:

Similarly, don't make every single selector memoized!. Memoization is only needed if you are truly deriving results or if the derived results would likely create new references every time. A selector function that does a direct lookup and return of a value should be a plain function, not memoized.

// ✅ SHOULD memoize: deriving data, so memoize to avoid pointless recomputations
const selectItemsTotal = state => {
  return state.items.reduce((result, item) => {
    return result + item.total
  }, 0)
}
const selectAllCompleted = state => state.todos.every(todo => todo.completed)
@EskiMojo14
Copy link
Contributor

EskiMojo14 commented Mar 17, 2025

I'm inclined to agree that it's a little less black and white than that snippet makes it out to be. Generally I go by:

  • selector will return a new object reference -> always memoize
  • selector derives a primitive value, or a stable lookup (e.g. .find) -> consider memoization
    • consider that sometimes the cost of checking whether we should recalculate is more expensive than just recalculating
  • direct lookup (e.g. state.foo.bar) -> never memoize

@jkillian
Copy link
Author

jkillian commented Mar 17, 2025

@EskiMojo14 I think that's reasonable! I lean a bit more conservatively and generally think that anything that's operating over an array should be memoized since you end up with a O(<num of components using selector> * <length of array>) computation, but if you're dealing with short arrays I imagine the cost of memoization could easily end up higher than just re-doing the computation. (Though maybe even in that case for simplicity and future-proofing it's better to just recommend memoization?)

@markerikson
Copy link
Contributor

Though maybe even in that case for simplicity and future-proofing it's better to just recommend memoization?

That's actually exactly what this section of the doc is trying to warn against.

It's much like the "Should I wrap every component in React.memo?" question. Yes, you can. Yes, doing that will ensure you get the benefits. But it's a lot of extra work that is often unnecessary, and it also leads to not understanding the point of why you're doing this in the first place.

The nuances of the snippet here are less important than getting across the concept that "sometimes a simple plain unmemoized function is all you actually need here".

@jkillian
Copy link
Author

That's fair, but I'd still argue @EskiMojo14's framing is an improvement here - telling people not to memoize expensive computations just because they return a scalar will often be detrimental advice. I can buy that "SHOULD memoize" in my OP may be too strong though.

@markerikson
Copy link
Contributor

markerikson commented Mar 17, 2025

Yeah, I think those are reasonable guidelines to have in here. (I still don't think "a bunch of additions" actually qualifies as "expensive" per se, but I get your overall point.)

@jkillian jkillian linked a pull request Mar 18, 2025 that will close this issue
2 tasks
@jkillian
Copy link
Author

Thanks both, put up a small PR to tweak the wording here: #4784.

I'm also not sure how many additions would have to be done to be actually expensive, probably quite a lot 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants