-
Notifications
You must be signed in to change notification settings - Fork 47.1k
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 hook #18000
useMutableSource hook #18000
Conversation
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) |
0f7cf73
to
560acda
Compare
57c32b1
to
bf33e75
Compare
bf33e75
to
26c85ca
Compare
fiber: Fiber, | ||
expirationTime: ExpirationTime, | ||
) { | ||
function scheduleUpdateOnFiber(fiber: Fiber, expirationTime: ExpirationTime) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This export was never referenced (only the scheduleWork
alias below) so I removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind you removing scheduleWork
instead? 😆 I meant to rename all the callsites to the more specific name during the last refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
26c85ca
to
17bc587
Compare
This PR is ready for review. I've updated it to reflect the latest thinking about the API. |
17bc587
to
ccef29a
Compare
I do not understand the Circle CI |
|
||
isSafeToReadFromSource = | ||
pendingExpirationTime === NoWork || | ||
pendingExpirationTime >= expirationTime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
useMutableSource
hooks. - 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the config changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
if (__DEV__) { | |
if ( | |
context._currentRenderer !== undefined && | |
context._currentRenderer !== null && | |
context._currentRenderer !== rendererSigil | |
) { | |
console.error( | |
'Detected multiple renderers concurrently rendering the ' + | |
'same context provider. This is currently unsupported.', | |
); | |
} | |
context._currentRenderer = rendererSigil; |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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. |
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