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

useMutableSource #147

Merged
merged 7 commits into from Mar 11, 2020
Merged

useMutableSource #147

merged 7 commits into from Mar 11, 2020

Conversation

bvaughn
Copy link
Collaborator

@bvaughn bvaughn commented Feb 14, 2020

useMutableSource() enables React components to safely and efficiently read from a mutable external source in Concurrent Mode. The API will detect mutations that occur during a render to avoid tearing and it will automatically schedule updates when the source is mutated.

View formatted RFC

Implementation: facebook/react#18000

[reduxSource]
);

const state = useMutableSource(reduxSource, config);
Copy link
Collaborator Author

@bvaughn bvaughn Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this example shows direct usage of useMutableSource, although this would likely be abstracted within a Redux-supplied custom hook (rather than used directly).

Redux would likely not expose the underlying source object returned by createMutableSource, but would instead perhaps expose some selector API (and perhaps a way to setup a scoped subscription).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like, say, uh.... useSelector()? :)

https://react-redux.js.org/api/hooks#useselector

Copy link
Collaborator Author

@bvaughn bvaughn Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you manage a scoped subscription for useSelector? Would it be possible? (I'm not familiar with Redux really.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on what we mean by "scoped".

Since Redux is a global store, any component can choose to extract any original or derived data from the store at any time.

As a typical example, a <UsersList> component might do:

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

While a <UserDetails> component might do:

const currentUser = useSelector(state => state.users.find(user => user.id === props.id);

Any component can have many useSelector calls at the same time.

So, it's "scoped" in that a given useSelector call only cares about certain pieces of data, but not scoped in that any useSelector can access any part of the state tree.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Managing a scoped subscription for useSelector should be possible. Redux can store the the scheduleUpdate callback along with the selector as a pair and iterate over them when the store updates. It will have to re-run the selectors to see if the derived values have changed but react-redux is already doing that.

I think there could be a problem with the selector not deriving its returned value from the snapshotted store but I think that can be solved by storing the last result from the getSnapshot function in redux land.

Copy link
Collaborator Author

@bvaughn bvaughn Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on what we mean by "scoped".

Fair enough. The idea was that stores like Relay and (maybe) Redux may be able to further optimize with useMutableSource to only schedule updates for components whose snapshot values change.

Given your example above, both <UsersList> and <UserDetails> read from state.users and so, conceptually, they wouldn't need to re-render when state.somethingElse changes.

So, it's "scoped" in that a given useSelector call only cares about certain pieces of data, but not scoped in that any useSelector can access any part of the state tree.

I think this is the key detail. If there's a way to constraint the scope/access a selector has, then the subscription could take advantage of this and avoid scheduling unnecessary no-op updates.

I think @JeremyRH is suggesting that Redux could automatically re-evaluate selectors when the store changes, and dispatch a scoped "change" event only if their value changes. Not sure how expensive this would be, since it would require all selectors to be eagerly evaluated.

It also seems that such a comparison might fail for derived cases like @markerikson shows above with <UserDetails>.

@markerikson
Copy link

@markerikson markerikson commented Feb 14, 2020

Great work, thanks for putting this together!

First question off the top of my head:

Are comparisons and tracking done only for the same consistent "source" object returned by createMutableSource? In other words, if I call createMutableSource(store) twice in a row with the same store reference, will the sources be considered equivalent and mixable, or do all calls to useMutableSource(source) need to use a === source value to work correctly amongst themselves?

@bvaughn
Copy link
Collaborator Author

@bvaughn bvaughn commented Feb 14, 2020

The mutable source object returned by createMutableSource can be thought of like a context object, in that that specific object should be passed to all useMutableSource hooks that share it (all that need to render together without tearing).

Maybe this means creating one at the module level. Maybe it means sharing it via context. Depends on the use case I suppose.

If you were to create two sources, e.g.

const source1 = createMutableSource(store);
const source2 = createMutableSource(store);

Then tearing would be possible between them.

text/0000-use-mutable-source.md Outdated Show resolved Hide resolved
@markerikson
Copy link

@markerikson markerikson commented Feb 14, 2020

Yeah, I figured as much.

More questions and thoughts:

  • I see the "selector" mention with getSnapshot. Does getSnapshot need to be a memoized function itself for correct behavior (ie, returning a consistent reference if its output hasn't changed)? Does getSnapshot returning a new value force a re-render of that component? Mmmm... looking at the non-Redux example, I'm guessing it doesn't need to be memoized, because it's doing a .map()
  • React-Redux currently requires React 16.8+, for hooks support. We had to bump a major to v7 to do that, and I'd really rather not bump another major for a while. I'm wondering how feasible it would be to feature-detect this and switch between our own existing subscription implementation and this one, depending on what React version is in use.
  • I'm trying to understand the subscription and event triggering flow here. Am I correct that (in the Redux example case):
    1. the hook itself ends up subscribing to the store, using a hook-provided callback passed to store.subscribe()
    2. when the callback is triggered, the hook will call getVersion() and retrieve the latest store state
    3. The hook then loops over nested "subscribers" defined via useMutableSource usages, and calls all of their getSnapshot() methods itself automatically (?)
    4. any calls to getSnapshot that return a new reference force a re-render (?)

If points 3 and 4 correct, then I have concerns about the perf characteristics here. That suggests to me that every Redux action dispatch will force some kind of a React render cycle just to call the getSnapshot methods so it can determine what components if any need to re-render. That sounds rather like the behavior characteristics we had in v6, minus the propagation of the store state value through context.

(I could be misreading this, I'm just trying to piece things together based on the first couple passes through.)

@tomByrer
Copy link

@tomByrer tomByrer commented Feb 14, 2020

This looks well thought out, but I'm still trying to wrap my mind around some part, including terms.

What is tearing?

Seems the issue you're trying to solve is keeping updated with data sources that have what some db call 'eventual parity' or what MeteorJS called 'optimistic UI' (show local state first, then update later with master remote data).
Does this sound about correct?

@markerikson
Copy link

@markerikson markerikson commented Feb 14, 2020

@tomByrer : "tearing" is when different components within the React tree read different versions of the same data during a single render pass, and thus render themselves inconsistently. For example, a Redux-connected component that dispatches an action in the middle of its constructor would potentially result in the upper half of the tree reading from state A, and the lower half from state B.

@sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Feb 14, 2020

This has nice performance characteristics. If you have many little subscriptions in a tree that need to add themselves to a Map and allocate space, you pay for that post-commit by using a passive effect. So this cost is paid in a non-critical path.

There's some cost paid for creating the config (especially with useMemo) and enqueueing the effect. This is within the realm of being optimizable in components though. In the cases that the config can be hoisted this has very nice performance characteristics to get to a full render pass and paint it. Then you pay for it afterwards to set things up.

I think that's the key benefit of a getVersion vs subscribing in render.

@JeremyRH JeremyRH mentioned this pull request Feb 14, 2020
2 tasks
@sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Feb 14, 2020

What do you think about dropping the names and wrapper config objects? None of the other built in hooks APIs have these (yet). It’s kind of nice for minification purposes but also avoids an extra object wrapper which would be nice. Especially in a compiler optimized form of useCallback memoization.

It also allows only the subscription callback to be hoisted. Eg if you have a subscription on the whole store but you need the closure only to get some of the data out in the snapshot.

const reduxSource = createMutableSource(store, () => reduxStore.getState());

function Example() {
  const getSnapshot = useCallback(store => store.getState(), [reduxSource]);
  const subscribe = useCallback((store, callback) => store.subscribe(callback)), [reduxSource]);
  const state = useMutableSource(reduxSource, getSnapshot, subscribe);

  // ...
}

@bvaughn
Copy link
Collaborator Author

@bvaughn bvaughn commented Feb 14, 2020

I don't have a strong preference one way or another on that, Seb. Let me think on it overnight 🙂

@dai-shi
Copy link

@dai-shi dai-shi commented Feb 14, 2020

Hi, for someone who might be interested,
I wrote imaginary code for use-context-selector.
Check this out: https://github.com/dai-shi/use-context-selector/blob/v2/src/index.ts
(This approach will not work for react-redux v7, in which we can't assume stable selectors.)

@mpeyper
Copy link

@mpeyper mpeyper commented Feb 14, 2020

Putting my library author hat on for a second, for the global store (Redux) use case, I don't quite see how we go from these primitives to the react-redux API (Sorry @markerikson for oversimplifying what this might look like):

// in user's code
const store = createStore(reducer);

function App() {
  return (
    <Provider store={store}>
      <ConnectedChildComponent />
    </Provider>
  );
}
// react-redux
const ReactReduxContext = React.createContext();

function Provider({ store, children }) {
  const config = useMemo(
    () => ({
      getSnapshot: store => store.getState(),
      subscribe: (store, callback) => store.subscribe(callback)
    }),
    [reduxSource] // how to go from store to mutable source?
  );

  const state = useMutableSource(reduxSource, config);

  const contextValue = useMemo(() = ({ state, store }), [state, store]);
  
  return (
    <ReactReduxContext.Provider value={contextValue}>
      {children}
    </ReactReduxContext.Provider>
  );
}

As my comment suggests, I'm failing to see how to pass the give the store to the Provider component in a way that allows it to abstract away the use of the mutable source.

I think this issue would be equally (if not more) painful if to implement with an API like the current react-redux implementation, as the store instance is accessed out of context, so it is also only available when rendering.

You could get around this by useMemoing the createMutableSelector call, but I feel like at this point, it feels like we're not using the tools as they were intended:

// react-redux
const ReactReduxContext = React.createContext();

function Provider({ store, children }) {
  const reduxSource = useMemo(() => createMutableSource(store, {
    getVersion: () => store.getState()
  }, [store]);

  const config = useMemo(
    () => ({
      getSnapshot: store => store.getState(),
      subscribe: (store, callback) => store.subscribe(callback)
    }),
    [reduxSource] // how to go from store to mutable source?
  );

  const state = useMutableSource(reduxSource, config);

  const contextValue = useMemo(() = ({ state, store }), [state, store]);
  
  return (
    <ReactReduxContext.Provider value={contextValue}>
      {children}
    </ReactReduxContext.Provider>
  );
}

I might be missing something, but I don't see any need to have the getVersion to be outside of the config object, and likewise, I'm failing to see what value createMutableSource is actually providing. What is preventing something like this from solving the problem in the same way?

// react-redux
const ReactReduxContext = React.createContext();

function Provider({ store, children }) {
  const config = useMemo(
    () => ({
      getVersion: store => store.getState(),
      getSnapshot: store => store.getState(),
      subscribe: (store, callback) => store.subscribe(callback)
    }),
    [store]
  );

  const state = useMutableSource(store, config);

  const contextValue = useMemo(() = ({ state, store }), [state, store]);
  
  return (
    <ReactReduxContext.Provider value={contextValue}>
      {children}
    </ReactReduxContext.Provider>
  );
}

I'm also wondering if there is any specific reason why the config object has to be memoized and if so, could that not be handled internally in useMutableSouce? I love it if the API could boil down to just:

// react-redux
const ReactReduxContext = React.createContext();

function Provider({ store, children }) {
  const state = useMutableSource(store, ({
    getVersion: store => store.getState(),
    getSnapshot: store => store.getState(),
    subscribe: (store, callback) => store.subscribe(callback)
  }));

  const contextValue = useMemo(() = ({ state, store }), [state, store]);
  
  return (
    <ReactReduxContext.Provider value={contextValue}>
      {children}
    </ReactReduxContext.Provider>
  );
}

My assumption here is that the first argument (store) could act as am implicit argument in the dependency array for a useCallback around the subscribe config option:

function useMutableSource(source, config) {
  // ... validation of arguments
 
  const { getVersion, getSnapshot } = config; // I don't think these ones need to be memoized, do they?
  const subscribe = useCallback(config.subscribe, [source]);

  // ... the rest of the implementation
}

As I said, I've probably missed something or my understanding of the whole thing is way off, so I won't be offended if you tear this idea to shreds.

@salvoravida
Copy link

@salvoravida salvoravida commented Feb 14, 2020

hi there!
the only concern i see is how to handle react-redux api like useSelector(state=> state.users[props.id])
dynamic created selectors from props.

while the version (some check if render or not) should be the selector result and not the state version itself)

in other words we can have 2 redux state but the selector result is the same.

how to handle this case without a memoized version of selector?

May i miss something?

@dai-shi
Copy link

@dai-shi dai-shi commented Feb 14, 2020

My gut feeling is it would be difficult (from my experience to apply use-subscription technique to react-redux reduxjs/react-redux#1509). For straightforward usage, we'd need to hit react-redux v8, which Mark said he'd want to avoid. There might be some workaround, though.

@salvoravida
Copy link

@salvoravida salvoravida commented Feb 14, 2020

currently react-redux

const user = useSelector(s=>s.users[props.id])

will re-render only if lastSelector(lastState)!==prevCalculatedSelectorState as selector may be inline

with current RFC of useMuableSource we need to memoize selector

so useSelector should be:

const useSelector = (getSnapshot, deps) => {
const { reduxSource, store } = useContext(ReduxContex)
 const config = useMemo(
    () => ({
      // Redux state is already immutable, so it can be returned as-is.
      // Like a Redux selector, this method could also return a filtered/derived value.
      getSnapshot,

      // Redux subscribe method already returns an unsubscribe handler.
      subscribe: (store, callback) => store.subscribe(callback)
    }),
    [reduxSource, ...deps]
  );

 return useMutableSource(reduxSource, config);
}
// use

const user= useSelector(state=>state.users[pros.id], [props.id])

is it not possible to invalidate config not on nextConfig!==currentConfig but on the execution of
nextConfig.getSnapshot(nextStoreVersion)!==currentConfig.getSnapshot(currentStoreVersion) ?
so useSelector can continue to have simple api without array deps?

this condition also should be more performant, as selector may changes,but result value No.

@dai-shi
Copy link

@dai-shi dai-shi commented Feb 14, 2020

As far as I understand, even if you get a next snapshot that is ref equal to the previous snapshot, that won't bail out rendering. You need a "scoped" subscription.
If my understanding is correct, here's what I've got.
https://github.com/dai-shi/use-context-selector/blob/23229108afa971afdc92cc074256c61ba921ffee/src/index.ts#L85-L92
Actually, I might hope my understanding is incorrect...

@markerikson
Copy link

@markerikson markerikson commented Feb 14, 2020

Yeah, I'm not yet understanding what the actual "trigger a re-render" condition is here. Is it when the root version changes, or based on comparisons of the return value from getSnapshot, or any time the callback argument gets called, or something else?

@markerikson
Copy link

@markerikson markerikson commented Feb 14, 2020

Also, while I haven't had time to dig through this thoroughly, my early gut reaction is that this hook may not fit well into the constraints we have with React-Redux as far as top-down update propagation. Per recent discussion over in facebook/react#17970 , our implementation for connect in particular relies on a nested series of re-renders in the commit phase so that child components only re-render once their nearest parent has re-rendered, to ensure that they have access to the latest props in mapState if necessary. I'm not sure how that ties into the way useMutableSource is intended to work.

Then again, it's also possible that connect may be inherently unable to use this due to its constraints, but useSelector might be a better fit as it has less internal complexity, and thus we might end up in a situation where a useSelector implementation based on useMutableSource becomes the recommended solution for best CM compat.

@bvaughn
Copy link
Collaborator Author

@bvaughn bvaughn commented Feb 14, 2020

Hi everyone. There are a few comments above, and GitHub doesn't allow me to respond to them in a threaded way, so please bear with me while I try to respond to them individually here.


First, @markerikson's comment.

I see the "selector" mention with getSnapshot. Does getSnapshot need to be a memoized function itself for correct behavior (ie, returning a consistent reference if its output hasn't changed)? Does getSnapshot returning a new value force a re-render of that component? Mmmm... looking at the non-Redux example, I'm guessing it doesn't need to be memoized, because it's doing a .map()

The config object (or the getSnapshot function itself, if the API changes to match @sebmarkbage's suggestion) must be memoized, yes. A change in this function/config suggests that the criteria for generating the snapshot has changed, and the previous snapshot is no longer safe to use. For example, if the snapshot does any kind of filtering using props (or hooks state) values, the getSnapshot function should change when (and only when) those values change.

React-Redux currently requires React 16.8+, for hooks support. We had to bump a major to v7 to do that, and I'd really rather not bump another major for a while. I'm wondering how feasible it would be to feature-detect this and switch between our own existing subscription implementation and this one, depending on what React version is in use.

You should be able to feature detect the existence of React.useMutableSource easily enough, but beyond that it is difficult to say since I'm not super familiar with the implementation details of react-redux.

I'm trying to understand the subscription and event triggering flow here. Am I correct that (in the Redux example case):

  1. the hook itself ends up subscribing to the store, using a hook-provided callback passed to store.subscribe()
  2. when the callback is triggered, the hook will call getVersion() and retrieve the latest store state
  3. The hook then loops over nested "subscribers" defined via useMutableSource usages, and calls all of their getSnapshot() methods itself automatically (?)
  4. any calls to getSnapshot that return a new reference force a re-render (?)

Numbers 2-4 are wrong, because they describe an eager evaluation.

When the callback is triggered, the hook will schedule an update with React for its component. (This happens for each component whose subscription handler is triggered.)

Neither the version number nor snapshot are not read at this time, but later during render. If the store changes again before React starts working on that render, this would not schedule an extra render. And if React needs to process a higher priority update first (e.g. a hover or an animation) it can do so reusing the previously cached "snapshot" value.

If points 3 and 4 correct, then I have concerns about the perf characteristics here. That suggests to me that every Redux action dispatch will force some kind of a React render cycle just to call the getSnapshot methods so it can determine what components if any need to re-render.

This is where the idea of "scoped" subscriptions comes in. If each component can only subscribe to the part of the store it cares about then it can avoid having any work scheduled for it when unrelated parts of the store change.

@filipemonteiroth
Copy link

@filipemonteiroth filipemonteiroth commented Feb 14, 2020

@bvaughn what would be the use case for that new hook that is not tied to react-redux/mobx? Could that help to avoid the context api tree dependency, when two context needs to communicate but they can't because if A is before B on the tree, B can talk to A, but A can't talk to B 🤔

@markerikson
Copy link

@markerikson markerikson commented Feb 14, 2020

@bvaughn : yeah, my question was about the results of getSnapshot, not the memoization of the actual getSnapshot function itself.

So it sounds like the key here is exactly when the callback argument of subscribe is called. If that is run, then the re-render is scheduled for that component.

For reference, React-Redux currently reads the store state and runs selectors inside of the store subscription, and triggers re-renders of a component if that selector value has changed.

Here's what a trimmed-down version of our useSelector hook looks like:

function useSelector(selector) {
    const [, forceRender] = useReducer( counter => counter + 1, 0);
    const {store} = useContext(ReactReduxContext);
        
    const selectedValueRef = useRef(selector(store.getState()));

    useLayoutEffect(() => {
        const unsubscribe = store.subscribe(() => {
            const storeState = store.getState();
            const latestSelectedValue = selector(storeState);

            if(latestSelectedValue !== selectedValueRef.current) {
                 selectedValueRef.current = latestSelectedValue;
                 forceRender();
            }
        })
        
        return unsubscribe;
    }, [store])

    return selectedValueRef.current;
}

Given that, it sounds like we might be able to call useMutableSource(), do the initial selector comparison in the subscribe function, run callback() to queue a re-render if the selector value changed, and then return the selector value (or re-run the selector) in the getSnapshot function.

@filipemonteiroth : any other event-based API would probably work, per the examples in the RFC document.

@markerikson
Copy link

@markerikson markerikson commented Feb 14, 2020

@bvaughn : another question. How does this play in to React's batching behavior? I assume the same current rules apply, no changes here?

  • Renders queued in React event handlers are batched due to React wrapping them with batchedUpdates()
  • Renders queued outside are not batched in standard mode
  • Renders queued outside will presumably be batched over time in CM

@bvaughn
Copy link
Collaborator Author

@bvaughn bvaughn commented Feb 14, 2020

Next, @mpeyper's comment.

I might be missing something, but I don't see any need to have the getVersion to be outside of the config object, and likewise, I'm failing to see what value createMutableSource is actually providing. What is preventing something like this from solving the problem in the same way?

Hoisting getVersion to the shared mutable source object ensures that the version number it returns is shared by all hooks that read from that source. This is an important design constraint for this API to avoid tearing. If getVersion were defined for each caller of useMutableSource, it's possible that different "versions" would be returned.

Given that the version must be for the entire source, the decision to formalize this by wrapping the source with createMutableSource makes more sense. (It also allows us to store things like the work-in-progress version number directly on this source object, rather than in a map, which makes reads/writes a little faster.)

I'm also wondering if there is any specific reason why the config object has to be memoized and if so, could that not be handled internally in useMutableSouce

Check out my response to @markerikson above:

The config object (or the getSnapshot function itself, if the API changes to match @sebmarkbage's suggestion) must be memoized, yes. A change in this function/config suggests that the criteria for generating the snapshot has changed, and the previous snapshot is no longer safe to use. For example, if the snapshot does any kind of filtering using props (or hooks state) values, the getSnapshot function should change when (and only when) those values change.

@bvaughn
Copy link
Collaborator Author

@bvaughn bvaughn commented Mar 11, 2020

Initial implementation just landed (behind a feature flag) in the main React repo, so I'm going to merge this RFC. There's more follow up to do (e.g. see facebook/react#18183) but an initial version is now ready to test in the experimental release channel. Thanks for all the input folks!

By the way @mhuebert, getVersion is passed the underlying source param now.

It looks like React considers that state private - ReactCurrentDispatcher and ReactCurrentOwner are inside a thing called .__SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED. I’m curious if there might ever be a public/approved way to get that information

This is definitely private API.

@bvaughn bvaughn merged commit 0cc6ea9 into reactjs:master Mar 11, 2020
@bvaughn
Copy link
Collaborator Author

@bvaughn bvaughn commented Mar 12, 2020

An experimental release has been published with useMutableSource in it. I made an over-simplified proof of concept react-redux example sandbox to be a reference for people wanting to start playing with it:
https://codesandbox.io/s/react-redux-usemutablesource-eyxoe

@dai-shi
Copy link

@dai-shi dai-shi commented Mar 12, 2020

I just published: https://www.npmjs.com/package/use-context-selector/v/2.0.0-alpha.1

npm install use-context-selector@next

There are two failing specs (02_tearing_spec and 03_stale_props_spec) I wish to overcome, but I don't think it's technically possible with useMutableSource.

@dai-shi
Copy link

@dai-shi dai-shi commented Mar 12, 2020

And also published: https://www.npmjs.com/package/react-tracked/v/2.0.0-alpha.1
I was hoping it to build on use-context-selector, but to support useTransition and state branching, I need to hack with scheduler (I'm not super confident with it, though. There's one issue for me.)

@everdimension
Copy link

@everdimension everdimension commented Mar 12, 2020

Hi! I have a question about the requirement for the snapshot to be immutable.
I.e. you say that this is okay:

getSnapshot: source => Array.from(source.friendIDs)

But what about this?

getSnapshot: source => Array.from(source.friends)

where source.friends is an array of objects. The RFC implies that it's not okay. If so, then I believe it's worth emphasizing.
But even then, I don't really get why it's necessary for the snapshot to be immutable. Isn't this what the version getter is for? So that parts of the source may be mutated, but react is informed about the fact that some mutation happened.


Another thing.
I haven't been using redux a lot lately, but from what I see the idea of the getSnapshot function seems to be identical to "selectors" commonly used in redux apps. Could it be a good idea to reuse an existing and understood term if it does the same thing?

@bvaughn
Copy link
Collaborator Author

@bvaughn bvaughn commented Mar 12, 2020

@everdimension Documentation will probably be more exhaustive in showing what is and isn't okay. Whether Array.from(source.friends) would be problematic depends on if anything could ever mutate one of the nested "friend" objects. If that's the case, then Array.from(source.friends) would not be safe. This hook can only prevent tearing if there are constraints.

Redux and Relay, for example, already employ immutable state so you can just return state.friends directly and not worry about it.

Selectors and the snapshot function are similar. getSnapshot has some more constraints though, like we're discussing. I don't know what terminology we'll end up using in the user-facing documentation. If you'd rather name the param a selector instead of getSnapshot - that's fine 😄 useMutableSource does not use named params, so you can call it whatever you want.

@markerikson
Copy link

@markerikson markerikson commented Mar 12, 2020

@bvaughn : per @slorber 's question on Twitter ( https://twitter.com/sebastienlorber/status/1238193228639285249 ), I'm not sure we ever talked about the order that the callbacks fire in.

Given a parent and a child, both calling useMutableSource with the same source: when the source updates, what order will the two components try to read the values from the source?

Our ongoing problem with React-Redux has been that children can subscribe before parents in some cases, so you can end up with situations such as "zombie children", where a child might try to read data from the store when the parent would actually have stopped rendering it (such as a list item being deleted), or reading data based on old props before the parent passed down new props ("stale props"):

https://react-redux.js.org/api/hooks#stale-props-and-zombie-children

https://gist.github.com/markerikson/faac6ae4aca7b82a058e13216a7888ec

I can vouch that for React-Redux, render batching has never been able to fix this, because our selectors are running outside of React. React-Redux v6 fixed this, but only because all our selectors ran inside of render(), so we naturally got React's top-down behavior for free.

@bvaughn
Copy link
Collaborator Author

@bvaughn bvaughn commented Mar 12, 2020

I also responded on Twitter:

Keep in mind that useMutableSource is being written for concurrent mode (not legacy sync-rendering mode).

So I don't think I understand why the order of subscriptions matters? The resulting React work should be batched into a single React update.

Filling in the missing dot (maybe?): If an update is scheduled for both the parent and child, React will render outside->in.

@dai-shi
Copy link

@dai-shi dai-shi commented May 4, 2020

@bvaughn Is there an issue to track this?

#147 (comment)

@dai-shi Ah, that's an interesting case I hadn't considered. Let me think on the best approach.

For now, you can work around this by returning a function that returns your function (similar to storing a function in useState hook)

@bvaughn
Copy link
Collaborator Author

@bvaughn bvaughn commented May 4, 2020

@bvaughn Is there an issue to track this?

No, I don't think so (unless you filed one?)

The mutable source implementation uses the same mechanism as useState, so a function value will get mistaken as an updater function.

I could use the updater function form instead to avoid this but that adds overhead for what is (I imagine) a pretty rare edge case. The workaround I mentioned (extra function wrapper) should work, just as it does with useState.

Please file an issue if you'd like it tracked explicitly.

@sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Nov 22, 2020

Since it might be buried a bit here. useMutableSource makes it so that you don’t have semantic breakages due to tearing. However it will still deopt all concurrent mode features when it happens. So it’s not compatible with new features.

It’s not recommended for new tooling that wants to be concurrent mode compatible. It’s for legacy compatibility.

We have some other ideas for mutable multi-version stores that might prove useful for systems that want to provide full future compatibility but in the current experimental releases the only way to be fully compatible is by using immutable data.

@mhuebert
Copy link

@mhuebert mhuebert commented Nov 22, 2020

it will still deopt all concurrent mode features when it happens

Does it deopt "always" or only in the (potentially rare) case when a mutable source changes during an update/render?

in the current experimental releases the only way to be fully compatible is by using immutable data

Perhaps I am being confused by the language here - do you mean "don't use useMutableSource at all", or "use useMutableSource only with immutable data structures"?

My particular use case is (still) how to use a ClojureScript atom with React. An atom is a kind of pointer to an immutable data structure - so the value we read at any given time (ie. what we can return from getSnapshot) is immutable, but the parent atom/source can be swapped to point to a new immutable thing at any time.

Or, a non-ClojureScript example - browser state like window.location.href - it's external state that can potentially change at any time, with an immutable (string) value, that we want to depend on from React views. Is useMutableSource an appropriate approach for that?

@orestis
Copy link

@orestis orestis commented Nov 22, 2020

Thanks @sebmarkbage for chiming in. What is the correct way then to handle global data that is actually immutable? I’m working with ClojureScript and I’ve implemented a store by generally copying the use-subscription package approach which is mainly backed by a useState hook.

I was under the impression that useMutableSource was the correct way for concurrent mode but you’re saying that I should keep using plain useState if my data in there is immutable?

@nickmccurdy
Copy link

@nickmccurdy nickmccurdy commented Nov 22, 2020

@orestis As long as you're keeping your data in React and updating it immutably with state, you shouldn't need useMutableSource, it's for external data sources.

@sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Nov 22, 2020

Does it deopt "always" or only in the (potentially rare) case when a mutable source changes during an update/render

Only when a source changes during an update/render.

Note that if something is paused such as with startTransition, or possible future features like staggering animations this might not be such a short time span. It can be many seconds.

If you use something like media queries or location.href it might be fine since it might be unlikely to happen very often at the same time.

Similarly if you use it to implement a data fetching library it might work better because you only get new data occasionally and round trips are slow which throttles the likelihood.

It’s more that if you use this to effectively replace all state in React you might have an increased likelihood to hit these pretty much all the time.

So if you’re implementing a state library that is meant to completely replace usages of useState, you might have a bad time.

I’m not sure how ClojureScript stores the atom. If it’s stored outside React then it’s a mutable field and that mutable field suffers from this.

However if your api is immutable it’s likely easier to build a multi-version atom that can support some new api.

A multi-version api can get full fidelity but requires some underlying support at the source to handle multiple versions. Similar to transactional databases.

@sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Nov 22, 2020

Perhaps I am being confused by the language here - do you mean "don't use useMutableSource at all", or "use useMutableSource only with immutable data structures"?

useMutableSource if you have either mutable data structures or if you have mutable atom where that atom lives outside React.

If you have immutable data, storing it in useState or useReducer is preferable because it will just work with everything.

@mhuebert
Copy link

@mhuebert mhuebert commented Nov 22, 2020

it’s likely easier to build a multi-version atom that can support some new api. A multi-version api can get full fidelity but requires some underlying support at the source to handle multiple versions. Similar to transactional databases.

this does sound like what I'm looking for.

I was also wondering if we could achieve something here via scheduling. Could a mutableSource offer a way to enqueue a function in which we can "safely" update the state? So that our state can live outside React (defined before any React root has been mounted, and used independently of any React views), but we let the React scheduler handle timing of updates?

Example usage might be a local in-memory db that we update from incoming websocket messages. Messages come in async anyway, and our app won't care or know if the timing is slightly adjusted so long as order is respected. Something like:

const websocketConnection = {..}
const memoryDB = atom()
const mutableSource = createMutableSource(memoryDB, getAtomValue)

websocketConnection.onmessage = (message) => {
  // could a mutable source let us schedule a function
  // in which we can safely update its state?
  mutableSource.schedule(() => memoryDB.swap(addMessageFn, message))
}

@dai-shi
Copy link

@dai-shi dai-shi commented Nov 22, 2020

We have some other ideas for mutable multi-version stores that might prove useful for systems that want to provide full future compatibility

Looking forward to this.

@samcooke98
Copy link

@samcooke98 samcooke98 commented Nov 22, 2020

Does it deopt "always" or only in the (potentially rare) case when a mutable source changes during an update/render

Only when a source changes during an update/render.

Note that if something is paused such as with startTransition, or possible future features like staggering animations this might not be such a short time span. It can be many seconds.

Sorry Seb, does "it can be many seconds" here mean that the update\render can be many seconds?

kodiakhq bot pushed a commit to vercel/next.js that referenced this issue May 8, 2022
- [x] Make sure the linting passes by running `yarn lint`

Back in 2019, React released the first version of `use-subscription` (facebook/react#15022). At the time, we only has limited information about concurrent rendering, and #9026 add the initial concurrent mode support.

In 2020, React provides a first-party official API `useMutableSource` (reactjs/rfcs#147, facebook/react#18000):

> ... enables React components to safely and efficiently read from a mutable external source in Concurrent Mode.

React 18 introduces `useMutableSource`'s replacement `useSyncExternalStore` (see details here: reactwg/react-18#86), and React changes `use-subscription` implementation to use `useSyncExternalStore` directly: facebook/react#24289

> In React 18, `React.useSyncExternalStore` is a built-in replacement for `useSubscription`.
> 
> This PR makes `useSubscription` simply use `React.useSyncExternalStore` when available. For pre-18, it uses a `use-sync-external-store` shim which is very similar in `use-subscription` but fixes some flaws with concurrent rendering.

And according to `use-subscription`:

> You may now migrate to [`use-sync-external-store`](https://www.npmjs.com/package/use-sync-external-store) directly instead, which has the same API as `React.useSyncExternalStore`. The `use-subscription` package is now a thin wrapper over `use-sync-external-store` and will not be updated further.

The PR does exactly that:

- Removes the precompiled `use-subscription` introduced in #35746
- Adds the `use-sync-external-store` to the dependencies.
  - The `use-sync-external-store` package enables compatibility with React 16 and React 17.
  - Do not pre-compile `use-sync-external-store` since it is also the dependency of some popular React state management libraries like `react-redux`, `zustand`, `valtio`, `@xstate/react` and `@apollo/client`, etc. By install
- Replace `useSubscription` usage with `useSyncExternalStore` 

---

Ref: #9026, #35746 and #36159


Co-authored-by: Jiachi Liu <4800338+huozhi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet