useMutableSource hook #18000
useMutableSource hook #18000
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
bvaughn
force-pushed
the
useMutableSource
branch
from
bd870a6
to
baad191
Compare
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit dee1164:
|
Details of bundled changes.Comparing: 30a998d...dee1164 react-dom
react-native-renderer
react-art
react-test-renderer
react-reconciler
react
react-debug-tools
ReactDOM: size: 🔺+0.3%, gzip: 🔺+0.2% React: size: 0.0%, gzip: -0.0% Size changes (stable) |
Details of bundled changes.Comparing: 30a998d...dee1164 react
react-dom
react-native-renderer
react-art
react-test-renderer
react-reconciler
react-debug-tools
ReactDOM: size: 0.0%, gzip: -0.1% React: size: 🔺+1.7%, gzip: 🔺+1.3% Size changes (experimental) |
bvaughn
force-pushed
the
useMutableSource
branch
2 times, most recently
from
0f7cf73
to
560acda
Compare
bvaughn
force-pushed
the
useMutableSource
branch
from
57c32b1
to
bf33e75
Compare
bvaughn
force-pushed
the
useMutableSource
branch
from
bf33e75
to
26c85ca
Compare
| fiber: Fiber, | ||
| expirationTime: ExpirationTime, | ||
| ) { | ||
| function scheduleUpdateOnFiber(fiber: Fiber, expirationTime: ExpirationTime) { |
There was a problem hiding this comment.
This export was never referenced (only the scheduleWork alias below) so I removed it.
There was a problem hiding this comment.
Do you mind you removing scheduleWork instead? 😆 I meant to rename all the callsites to the more specific name during the last refactor.
bvaughn
force-pushed
the
useMutableSource
branch
from
26c85ca
to
17bc587
Compare
|
This PR is ready for review. I've updated it to reflect the latest thinking about the API. |
bvaughn
force-pushed
the
useMutableSource
branch
from
17bc587
to
ccef29a
Compare
bvaughn
changed the title
|
I do not understand the Circle CI |
|
|
||
| isSafeToReadFromSource = | ||
| pendingExpirationTime === NoWork || | ||
| pendingExpirationTime >= expirationTime; |
There was a problem hiding this comment.
Note to self: If this component is newly mounting, we need to always check the version number as well, to account for the case of tearing between newly mounted components that have not yet subscribed to the store (and so may miss a tear).
I'll add a test for this (and update the code) tomorrow.
There was a problem hiding this comment.
Related: if the source version matches the work-in-progress version, then you don't need to check if the current render is inclusive of the pending mutation updates. Because that check would only fail if there was a mutation since the work-in-progress version was originally set, in which case the the version would have been bumped.
In other words, you only have to check for pending mutation updates if there's not already a work-in-progress version, i.e. if this is the first mutable read of this source during this render. For subsequent reads, the version alone is sufficient to tell if there was a mutation.
There was a problem hiding this comment.
Yes, this sounds right. I could avoid checking the expiration time if I already have a WIP version. Good suggestion.
There was a problem hiding this comment.
Great work! (Sorry for the avalanche of comments! Because the high level approach is essentially correct, my feedback is more detailed than usual for a first review pass.)
Some of my feedback is a bit nitpicky, but I really just have one big note: we should fork the mount and update implementations. They have different trade offs. (By "mount" I mean both new useMutableSource hooks and when the source or config changes, i.e. "mount" in the useEffect sense of the word.)
For renders that don't include a new hook, we can support full concurrency with no de-opts.
To illustrate what I mean, here is an additional test case to consider — multiple pending mutations at different priorities, no new mounts:
- Render a tree that includes multiple
useMutableSourcehooks. - Mutate one or more of the sources at high priority.
- Before processing the update, mutate again at a lower priority.
- Now flush the work.
- There should be two commits: the high priority mutation, followed by the low priority mutation.
The way you would implement this is with a state queue. You can use the one provided by useState/useReducer. This works because for mutations after mount, we can call getSnapshot in the event that triggers the mutation instead of in the render phase. Then we add that snapshot to the queue. At that point, they're exactly like normal updates. If the source is mutated before the update commits, that's fine because we already took the snapshot. If there are multiple snapshots, that's also fine because each one of them is in the queue.
It's only in the mount case where we're forced to read during the render phase, which necessitates the extra consistency checks.
| fiber: Fiber, | ||
| expirationTime: ExpirationTime, | ||
| ) { | ||
| function scheduleUpdateOnFiber(fiber: Fiber, expirationTime: ExpirationTime) { |
There was a problem hiding this comment.
Do you mind you removing scheduleWork instead? 😆 I meant to rename all the callsites to the more specific name during the last refactor.
|
|
||
| hook.memoizedState = memoizedState; | ||
|
|
||
| if (prevSource !== source) { |
There was a problem hiding this comment.
What if the config changes?
There was a problem hiding this comment.
Interesting question. My thinking had been that a change in config would really just be a change in getSnapshot dependencies, and not a change in subscribe. Sebastian's suggestion on the RFC to get rid of the wrapper config object and pass both callbacks directly would make this more explicit, in which case I will add a check for that here as well.
| isPrimaryRenderer: boolean, | ||
| ): void { | ||
| if (isPrimaryRenderer) { | ||
| mutableSource._workInProgressVersionPrimary = version; |
There was a problem hiding this comment.
In the context implementation, we fire a warning if we detect multiple primary renderers.
react/packages/react-reconciler/src/ReactFiberNewContext.js
Lines 86 to 97 in 2d6be75
We should probably do the same here. Although I think it will need to go in getWorkInProgressVersion instead of set.
| ); | ||
|
|
||
| // If an update is already scheduled for this source, re-use the same priority. | ||
| if (alreadyScheduledExpirationTime !== undefined) { |
There was a problem hiding this comment.
This check means that if you update at one priority, then again at a different priority, the first priority always wins. We should support multiple concurrent priorities, which is solved by using the useState queue. (The test case I described in my other comment would cover this.)
There was a problem hiding this comment.
Yes, my thinking on this was that scheduling more than one update is only valid if we eagerly read snapshot values. I think the key question is whether or not that's a positive change. If it is, then going with an update queue makes sense.
|
The lint build error is legit. I looked at the ReactFabric-dev artifact and found this: // ReactFabric-dev.js:11656
useMutableSource: function(source, config) {
currentHookNameInDev = "useMutableSource";
mountHookTypesDev();
return Snapshot > config;
}Looks like some sort of build error related to Flow types? Phew that we have the lint build task! Also an argument for why we should be running more of our tests against the bundles (and therefore moving away from |
bvaughn
force-pushed
the
useMutableSource
branch
from
ccef29a
to
9968cf1
Compare
That's interesting! 😄 I initially assumed it was a transient issue yesterday. Just hadn't had the chance to dig into it yet today. |
bvaughn
force-pushed
the
useMutableSource
branch
from
55b0b1a
to
381939d
Compare
This prevents the subscription callback from closing over stale values.
Even if the current snapshot value is the same, there may be pending, lower priority updates that we no longer want to eventually render.
I initially thought that we could treat them as safe if the returned snapshot value was the same, but this ignored the case where the underlying source was mutated between when the state update was scheduled and when the component later rendered with a new getSnapshot function. (This commit includes a test that failed without this fix.)
1. Reset mutable source WIP version in complete and unwind work (rather than prepare fresh stack). 2. Add check to warn about multiple primary/secondary renderers using the same mutable source. (Patterned after our current context warning.) 3. Changed the way I'm calculating expiration time for mutated sources to use computeExpirationForFiber(). 4. Changed undefined unsubscribe function from invariant to warning. 5. Replaced !== checks with Object.is() checks to be more consistent with precedent. 6. Added a couple of new warning tests. (One of them has a pending TODO.)
1. Use currentlyRenderingFiber to calculate expiration time rather than using a ref. 2. Use polyfilled is() rather than Object.is() 3. Add __EXPERIMENTAL__ guard to test since new APIs aren't in stable build 4. Removed error code that was changed to warning.
|
I rebased my PR on top of #18265 to see if it fixes the failing test I added recently. It does! 👍 I realized that there's another multi-renderer case though that we weren't handling properly: when a source is used by one renderer, then mutated, then used by another renderer. To be clear, this isn't supported, but it currently throws a user visible error. That's because when a change of version is detected, React throws and unwinds- and is supposed to reset all WIP version numbers. This is done by looping through the mutable sources that were updated during the current render and resetting them. In this case though, we never updated the torn source for the second renderer (only read from it) so React doesn't know to reset its version while unwinding. (This means the re-render attempt will encounter the same error, and React will give up.) I think we have a couple of options here:
Edit for clarity None of the above "solutions" are great. Even the one I chose to go with for now can lead to tearing in the first renderer when it resumes. I think it's the best we can do though, if we want to avoid the invariant. |
If a mutation happens between the first and second renderer to use a source, we need to explicitly reset the source before restarting the second renderer.
bvaughn
force-pushed
the
useMutableSource
branch
from
a6d4eed
to
45ed506
Compare
I am being a little lazy here and merging instead of rebasing because practically every commit conflicted with Dominic's recent useEvent PR. If anyone feel strongly about this, I will revert the commit and rebase. Since the commit will be squashes away during the merge though, I don't think it matters much at this point.
|
I think this is fine. We are intentionally not supporting anything more than a primary renderer and a secondary renderer, to keep the simpler implementation. And the reason to use a warning instead of an error is so the check is stripped out in prod. |
|
Right. That was my thinking too. Okay then. Good to go? |
|
Is there gonna be any documentation about how to use this hook on the React docs site? |
|
Once it's in a stable release, yes. This is still only available in the experimental React release channel. |
- [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>
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.RFC: reactjs/rfcs#147