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