Skip to content

Potentially misleading advice in "Balanced Selector Usage" #4783

Open
@jkillian

Description

@jkillian

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)

Activity

EskiMojo14

EskiMojo14 commented on Mar 17, 2025

@EskiMojo14
Contributor

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

jkillian commented on Mar 17, 2025

@jkillian
Author

@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

markerikson commented on Mar 17, 2025

@markerikson
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

jkillian commented on Mar 17, 2025

@jkillian
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

markerikson commented on Mar 17, 2025

@markerikson
Contributor

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 commented on Mar 18, 2025

@jkillian
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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @markerikson@jkillian@EskiMojo14

      Issue actions

        Potentially misleading advice in "Balanced Selector Usage" · Issue #4783 · reduxjs/redux