useMutableSource and selector stability

[markerikson]

Background

The React team has created the useMutableSource hook as a compatibility mechanism to allow external state stores like Redux to avoid issues with "tearing". The behavior and implementation of useMutableSource is described in these references:

• RFC: useMutableSource (description)
• RFC: useMutableSource (discussion)
• PR #18000: useMutableSource

A couple days ago, some concerns were raised on Twitter about the apparent need to memoize the creation of selector functions via useCallback, starting in this thread: https://twitter.com/TkDodo/status/1427922647401312257. I later got pulled into the discussion further down, starting at https://twitter.com/TkDodo/status/1428063106450743298 .

The concern is that if a library migrates to using useMutableSource internally, all "selector" functions written by users of that library will now have to be wrapped in useCallback or useMemo to ensure that the selector functions have stable references. This appears to be necessary because useMutableSource relies on function reference changes as an indication that the current snapshot value needs to be recalculated, per the comment from Brian at https://twitter.com/brian_d_vaughn/status/1428076939466612740 :

If a selector fn changes, useMutableSource() doesn’t know if it's safe to reuse a snapshot value. (A new fn could select a new value- either bc it closes over new props or bc the actual function is totally different). You can memoize with useCallback.

This pattern is actually already demonstrated in the hypothetical sample code in the RFC at https://github.com/reactjs/rfcs/blob/master/text/0147-use-mutable-source.md#example-user-component-code :

import { useSelector } from "react-redux";

function Example() {
  // The user-provided selector should be memoized with useCallback.
  // This will prevent unnecessary re-subscriptions each update.
  // This selector can also use e.g. props values if needed.
  const memoizedSelector = useCallback(state => state.users, []);
  
  // The Redux hook will connect user code to useMutableSource.
  const users = useSelector(memoizedSelector);

  // ...
}

Daishi Kato has done some experimenting with useMutableSource, and in a proof of concept concluded that unstable selector references could cause a form of infinite loop: reduxjs/react-redux#1351 (comment) . However, he also just said he made a newer experiment and there may not have been as much of an issue: https://twitter.com/dai_shi/status/1428198999954132992 .

Concerns

While this is a very understandable implementation approach, the need to memoize all selector functions appears to be a rather painful change to how libraries like React-Redux are currently used. For example, it's very common for selector functions to be defined as inline arguments to the useSelector hook, such as these typical snippets:

const todos = useSelector(state => state.todos)

const todo = useSelector(state => selectTodoById(state, id))

This is especially true if the selectors reference values from props.

While it's not impossible for our users to wrap all selectors in useCallback, this would potentially be a sizeable breaking change for users who want to upgrade to React-Redux v8, as well as for users of other state libraries that have to go through similar changes - there's a lot of inline selectors in codebases out there.

This is also a bit of a change conceptually, in that the current React docs have emphasized that "useCallback and useMemo are only there for perf optimizations". I do understand the implementation issues here, but it's definitely a shift in intended usage.

For reference, React-Redux's useSelector hook also faces the same concern with new selector references changing. We deal with that by re-running the selector at render time if it's a new reference, but reusing the existing state value if the old and new values pass an equality check, per https://github.com/reduxjs/react-redux/blob/v7.2.4/src/hooks/useSelector.js#L31-L48 :

    if (
      selector !== latestSelector.current ||
      storeState !== latestStoreState.current ||
      latestSubscriptionCallbackError.current
    ) {
      const newSelectedState = selector(storeState)
      // ensure latest selected state is reused so that a custom equality function can result in identical references
      if (
        latestSelectedState.current === undefined ||
        !equalityFn(newSelectedState, latestSelectedState.current)
      ) {
        selectedState = newSelectedState
      } else {
        selectedState = latestSelectedState.current
      }
    } else {
      selectedState = latestSelectedState.current
    }

Questions

With all that info, some questions:

• What is the expected behavior if non-stable selector function references are passed in to useMutableSource on each render?
• Is there a way to alter the implementation of useMutableSource so that it's more resilient to changing selector references?
• Is there something that you would suggest doing at the library level to avoid users having to alter large portions of their selector usages?
• If nothing can be done to change the behavior, how should we approach communicating this change to users given the potential pain in migrating to a React-18-compat version of libraries like React-Redux?

Tagging some interested parties for awareness:

@bvaughn , @gaearon , @TkDodo, @tannerlinsley, @phryneas , @timdorr, @dai-shi

[bvaughn]

Thank you for putting together this detailed summary, Mark. 🙇🏼

For reference, React-Redux's useSelector hook also faces the same concern with new selector references changing. We deal with that by re-running the selector at render time if it's a new reference, but reusing the existing state value if the old and new values pass an equality check, per https://github.com/reduxjs/react-redux/blob/v7.2.4/src/hooks/useSelector.js#L31-L48 :

    if (
      selector !== latestSelector.current ||
      storeState !== latestStoreState.current ||
      latestSubscriptionCallbackError.current
    ) {
      const newSelectedState = selector(storeState)
      // ensure latest selected state is reused so that a custom equality function can result in identical references
      if (
        latestSelectedState.current === undefined ||
        !equalityFn(newSelectedState, latestSelectedState.current)
      ) {
        selectedState = newSelectedState
      } else {
        selectedState = latestSelectedState.current
      }
    } else {
      selectedState = latestSelectedState.current
    }

I know this comment may add more controversy to and already controversial thread, but I think it's important for the discussion to be complete and take all things into consideration.

That being said, reading and writing from a ref during render like this is also unsafe. The rule of thumb for refs is:

• Reading or writing a ref value during render is only safe if you are implementing the lazy initialization pattern.
• Other types of reading are unsafe as the ref is a mutable source like any other.
• Other types of writing are unsafe as they are effectively side effects.

[bvaughn]

Does it make sense why reading from a mutable source (like location.hash or even Date.now()) during render is problematic? (Essentially the source can change while a render is in progress. Two components reading from the same source might get different values, causing the UI to "tear".)

A ref is really just a specific type of mutable source. (It's a stable object with a current property that gets mutated.)

[markerikson]

Yeah, I sorta get that. At the same time, I feel like there's a bunch of scenarios where that ends up being necessary one way or the other, and stamping that out across the ecosystem is going to be awfully hard to do.

[bvaughn]

Difficult, yeah. This is why the warning I added (facebook/react#18545) hasn't been turned on anywhere.

[gaearon]

We probably shouldn’t get sidetracked by this since, like Mark said, the goal is to remove this code anyway.

I’d add through that there is no goal to “stamp it out” immediately — which goes back to the previous discussions in which we mentioned that “in practice most things work anyway”. There is no goal to fix all patterns that theoretically can break across the ecosystem by the time 18 is out. We should do a better job of explaining how to use refs (noted for the new docs) but in general, for most of the ecosystem it makes sense to fix on a case by case basis. Like if someone actually runs into a problem. We don’t anticipate that every theoretical problem will become a practical problem.

[Andarist]

We don’t anticipate that every theoretical problem will become a practical problem.

Given React's scale - isn't every theoretical problem a practical one? I agree that the direct consumers of React (devs) won't see the difference and “in practice most things will work anyway”. I'm worried about end users though - I know that this might sound like an exaggeration but encouraging us not to worry about "theoretical" problems seems like a road for those problems to happen more often. I truly believe that it's not a question of if those things will happen but rather how often they will happen.

I know though that people were using things like refs in a way that abused their design (myself included). I'm not sure what's the best exit strategy in this situation - you all have been preaching about this stuff for a long time and yet most of us broke the given contract to implement different patterns. It's just troublesome for me that the proposed answer is to stop worrying (or worry less) about this kind of stuff because the outcome of those problems can be super random, we don't control it and the resulting breakage might vary between none to severe.

Personally, I would probably be more satisfied with an answer like "those things can sometimes happen - please go fix your code". I know that you don't say that we shouldn't fix our code but that used narrative can be taken this way (or in a similar) when read out of context etc.

[dai-shi]

Daishi Kato has done some experimenting with useMutableSource, and in a proof of concept concluded that unstable selector references could cause a form of infinite loop: reduxjs/react-redux#1351 (comment) . However, he also just said he made a newer experiment and there may not have been as much of an issue: https://twitter.com/dai_shi/status/1428198999954132992 .

A minor clarification. It's not infinite loop, but didn't work as expected. Probably, I confused it with the behavior of use-subscription.

[dai-shi]

There was a silly mistake in my codesandbox example. Forgot to use createRoot.
https://codesandbox.io/s/react-ums-with-inline-selector-b74r2 is updated.

(Now, only "Zustand Inline" doesn't work. I will investigate it. There might be some misunderstanding of mine.) Fixed with https://www.npmjs.com/package/zustand/v/4.0.0-alpha.1

[dai-shi]

Someone reported getting the error Cannot read from mutable source during the current render without tearing.
https://twitter.com/AlaaZorkane/status/1428985401239347201

[rickhanlonii]

I've been trying to repro and can't trigger it, but if anyone is able to repo please ping!

[acdlite]

Thanks for this post! We're working on a proposal that I think will address your concerns here. Aiming to share it by the end of the week.

The short version is: I do think we can remove the requirement that selectors need to be memoized by adding a selector option that separate from getSnapshot.

Then Redux's implementation of useSelector would look something like this:

function useSelector(selector) {
  const store = useContext(ReduxStore);
  return useMutableSource(store.subscribe, store.getState, {
    // Or whatever the API ends up being
    unstable_selector: selector,
  });
}

One thing we were hoping to avoid, though, is a isSelectionEqual API. For the reasons that we've discussed regarding context selectors (which is essentially the same design space).

So instead of

const {a, b} = useSelector(state => ({a: state.a, b: state.b}), shallowEqual);

users would write:

const a = useSelector(state => state.a);
const b = useSelector(state => state.b);

Is it feasible to remove support for isSelectionEqual from React Redux?

To support migration, we could also add temporary support in 18, then remove in 19. But if we could drop support sooner rather than later that'd be great. As a bonus this could move Redux one step closer to being able to switch to a context-based implementation.

[acdlite]

Having lots of transformation logic in selectors is a core part of Redux usage, and that definitely can't just be shifted.

Is it something y'all are open to changing, though? It's going to be hard to guarantee that Redux continues working with future features otherwise. useMutableSource might be the end of the line in terms of feature support — and even uMS is a compromise, because it will de-opt when there are concurrent updates.

It's not going to block 18 adoption but my concern is that Redux users are going to have a compromised experience going forward relative to newer state management libraries.

[markerikson]

It's not really even an "us" thing. It's a "millions of lines of existing code"-type thing.

Like I said, I'm open to discussion here, but there's got to be a very well-defined migration path that we can present as a long-term approach, because this is rapidly starting to sound like another "yeah, your class components are all dead, go rewrite to functions now" kind of a thing.

[acdlite]

Hooks are a good analogy because we didn't remove support for classes, we added a new API that created a new set of best practices.

Relay followed a similar strategy: when they added support for Suspense, they didn't delete the old HOC-based APIs, they added new ones that live side-by-side.

So I'm not suggesting a breaking change necessarily. It could be a new API that you encourage users (especially new users) to gradually start moving toward.

[markerikson]

One of the points I'm proud of with React-Redux is that we've had almost no API-level breaking changes over time. The only things we've really broken were a couple rarely used options around ref forwarding from v5-v6.

We introduced useSelector two years ago, and it's been fantastically well received. I would not want to have to start pushing users off to some new useSelector2ElectricBoogaloo API just to satisfy compat purposes. I know what you're saying about hook/class compat, but let's be realistic, it is a long-term breaking change to force people to rewrite code for compat.

[acdlite]

Yeah I'm sympathetic to the migration concerns. Let's set that conversation aside for now, though I will keep advocating for Redux to start thinking about how to shift away from expensive isEqual checks in the future.

In the short term, I think what we'll likely do is add an unstable_isSelectionEqual as an option to useMutableSource (although not useContext). It's really not an implementation burden for us to support that indefinitely. That would unblock migration to 18, which is really the most important goal with useMutableSource in general.

I think it's really unlikely we'll add support for this to our future APIs, though, like context selectors, since those rely on being able to bail out quickly without having to first do lots of computation. (So in Reselect terms, the "input selectors" are fine to run eagerly but the derivation logic would need to move to the hook/component body.) But we can discuss that elsewhere, post-18 perhaps.

[dai-shi]

#84 (comment)

The short version is: I do think we can remove the requirement that selectors need to be memoized by adding a selector option that separate from getSnapshot.

While I see uMS is mainly for library authors, I feel this api is confusing... It would become a technology debt.
As long as inline selectors work for level 2 and stable selectors increase performance, the community should migrate to stable selectors gracefully.
(a side note: this was really a concern when we discussed react-redux hook design.)

I have a question. Suppose, if we can afford to migrate to stable functions, can equalityFn support be implemented like this?

  const getSnapshot = useMemo(() => {
    let prevValue = null;
    return (state) => {
      const nextValue = selector(state);
      if (equalityFn(prevValue, nextValuue)) {
        return prevValue;
      }
      return (prevValue = nextValue);
    }
  }, [selector, equalityFn]);

Or, does it include an issue with concurrent rendering?

cc @acdlite @markerikson (created a different thread to avoid interrupting the discussion there.)

[bvaughn]

If the former is really okay, or even recommended, getSnapshot could be in createMutableSource, no?

Mutable source is meant to be global (shared throughout an application). getSnapshot (selector functions) are often based on component-local props or state.

This is mentioned in the RFC:
https://github.com/reactjs/rfcs/blob/master/text/0147-use-mutable-source.md#selectors-that-use-props

[acdlite]

It has too many options. scoped subscription, getSnapshot and now unstable_selector. unstable_selector seems like a real escape hatch.

Yeah I get that it's complicated but it's really designed to be used by a state management library. It's not meant for the typical component.

For example, another option that I haven't even mentioned yet relates to support for SSR hydration. That's another feature that we don't expect or want the typical user to have to think about, but something like Redux or Apollo definitely should.

[dai-shi]

Sorry, I didn't mean any critics here.
My focus was the question if the snippet of getSnapshot usage is valid.

  const getSnapshot = useMemo(() => {
    let prevValue = null;
    return (state) => {
      const nextValue = selector(state);
      if (equalityFn(prevValue, nextValuue)) {
        return prevValue;
      }
      return (prevValue = nextValue);
    }
  }, [selector, equalityFn]);

Or, is unstable_isSelectionEqual only the solution?

[acdlite]

It almost works, but there are a few edge cases where it wouldn't. The prevValue that you should compare to is the value that was most recently committed to the screen, whereas in this example prevValue is the value that was queued.

So an example of how it might break is if the source emits an update to state B inside startTransition. (Let's say you start with state A.) The components will start re-rendering with state B at transition priority. Then, before they finish, the source emits another update to state B, but this time as part of a discrete update. React should now render B at high priority, but because of the equality check, the subscription bails out without doing anything. It sounds theoretical but this is the type of subtle concurrency mistake that can lead to real world consequences.

Alternatively, you could store the prevValue value in a ref and update the ref with an effect. But this has the inverse problem: if instead you receive a high priority update back to A while B is already in progress, you need to enqueue another update to A even to override the update to B. Since React doesn't expose whether there are pending updates queued on a hook/component, the only way to correctly implement this feature is within React itself.

[dai-shi]

Thanks. I thought I understood it at first, then not sure. If selector changes, prevValue will be gone. I might misunderstand how useMemo works.

So, let's step back and see the example with memoization use case like reselect.

// outside render
const equalityFn = shallowEqual;
const selectFoo = (state) => [state.foo]; // unrealistic, but just as an example

let prevValue = null;
const memoizedSelectFoo = (state) => {
  const nextValue = selectFoo(state);
  if (equalityFn(prevValue, nextValuue)) {
    return prevValue;
  }
  return (prevValue = nextValue);
};

const Component = () => {
  const getSnapshot = useMemo(() => {
    return (store) => memoizedSelectFoo(store.getState());
  }, []) // this doesn't need to be in render though...
  return useMutableSource(mutableSource, getSnapshot, subscribe);
};

Does this still not work in the said edge cases?

[markerikson]

As an update for anyone reading this: the React and Redux teams just met to discuss issues around useMutableSource and selectors. I'll wait to see if the React team posts some notes from the discussion, but the general summary is that:

• useMutableSource will be reworked to always render synchronously, including a possible rename and some other API changes
• that simplifies many aspects of dealing with selectors, including potentially "stale" selectors and bailing out
• It's likely that an equality check will be built in because that would simplify several end-user cases
• The React team will publish a "shim" package to allow using this with both React 17 and React 18
• Most transitions are likely more associated with routing than with Redux state, so those would be still usable at the same time
• Most Redux apps shouldn't have to change at all as part of an upgrade to React 18 and React-Redux v8
• Redux is serving as a useful testbed for integration pain points, and whatever improvements are made to help React-Redux work with React 18 will then be applicable to other libs as well

[acdlite]

Great summary, Mark! Just a caveat regarding the first bullet: updates (i.e. dispatch) will trigger a synchronous render, but you can still read from Redux hooks/components during some other concurrent render, like one triggered by a router or useDeferredValue. Which means Redux can work side-by-side with concurrent features.

[dai-shi]

Thanks for the update. Just wondered if this rework can have anything to do with level 3 support.

#70

Another example is to use a mutable store with immutable snapshots which do not change during render, but this strategy is still under research.

I expected (hoped) the current uMS is somewhat close to level 3 support eventually. So, I wonder if the new one could become closer, farther, or unrelated.

[acdlite]

I shared some more details in a new discussion thread: #86

[acdlite]

I expected (hoped) the current uMS is somewhat close to level 3 support eventually. So, I wonder if the new one could become closer, farther, or unrelated.

Level 3 support is outside the scope of useMutableSource (which we're now calling useSyncExternalStore). We will have to introduce additional APIs for that. The most likely candidates we're considering are improvements to the context API, but we've postponed that work until after the initial React 18 release, for now.

[dai-shi]

Thanks for the clarification!