Allow updating dehydrated root at lower priority without forcing client render #24082
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: dbe9e73...110bbf0 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
acdlite
force-pushed
the
root-hydration-updates
branch
2 times, most recently
from
d80f909 to
7f8d81e
Compare
acdlite
commented
Mar 12, 2022
| @@ -245,9 +277,9 @@ describe('ReactDOMFizzShellHydration', () => { | |||
| root.render(<Text text="New screen" />); | |||
| }); | |||
| expect(Scheduler).toHaveYielded([ | |||
| 'New screen', | |||
There was a problem hiding this comment.
These switched order because the recoverable error gets logged during the commit phase instead of immediately in the event handler.
sebmarkbage
reviewed
Mar 17, 2022
| // If this was a forced client render, there may have been | ||
| // recoverable errors during first hydration attempt. If so, add | ||
| // them to a queue so we can log them in the commit phase. | ||
| upgradeHydrationErrorsToRecoverable(); |
There was a problem hiding this comment.
This is new. Was that a bug before?
There was a problem hiding this comment.
Oh, it's because of the new error case you added.
sebmarkbage
reviewed
Mar 17, 2022
| } else { | ||
| // The outermost shell has not hydrated yet. Start hydrating. | ||
| enterHydrationState(workInProgress); | ||
| if (supportsHydration) { |
There was a problem hiding this comment.
This should also be gated on enableUseMutableSource.
sebmarkbage
approved these changes
Mar 17, 2022
I already made this change for the concurrent root API in facebook#23309. This does the same thing for the legacy API. Doesn't change any behavior, but I will use this in the next steps.
Currently this does nothing except read a boolean field, but I'm about to change this logic. Since this is accessed by React DOM, too, I put the function in a separate module that can be deep imported. Previously, it was accessing the FiberRoot directly. The reason it's a separate module is to break a circular dependency between React DOM and the reconciler.
Currently, if a root is updated before the shell has finished hydrating (for example, due to a top-level navigation), we immediately revert to client rendering. This is rare because the root is expected is finish quickly, but not exceedingly rare because the root may be suspended. This adds support for updating the root without forcing a client render as long as the update has lower priority than the initial hydration, i.e. if the update is wrapped in startTransition. To implement this, I had to do some refactoring. The main idea here is to make it closer to how we implement hydration in Suspense boundaries: - I moved isDehydrated from the shared FiberRoot object to the HostRoot's state object. - In the begin phase, I check if the root has received an by comparing the new children to the initial children. If they are different, we revert to client rendering, and set isDehydrated to false using a derived state update (a la getDerivedStateFromProps). - There are a few places where we used to set root.isDehydrated to false as a way to force a client render. Instead, I set the ForceClientRender flag on the root work-in-progress fiber. - Whenever we fall back to client rendering, I log a recoverable error. The overall code structure is almost identical to the corresponding logic for Suspense components. The reason this works is because if the update has lower priority than the initial hydration, it won't be processed during the hydration render, so the children will be the same. We can go even further and allow updates at _higher_ priority (though not sync) by implementing selective hydration at the root, like we do for Suspense boundaries: interrupt the current render, attempt hydration at slightly higher priority than the update, then continue rendering the update. I haven't implemented this yet, but I've structured the code in anticipation of adding this later.
acdlite
force-pushed
the
root-hydration-updates
branch
from
7f8d81e to
110bbf0
Compare
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Mar 24, 2022
Summary: This sync includes the following changes: - **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([#24088](facebook/react#24088)) //<Sebastian Silbermann>// - **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([#24131](facebook/react#24131)) //<David McCabe>// - **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([#24082](facebook/react#24082)) //<Andrew Clark>// - **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([#24126](facebook/react#24126)) //<Sebastian Markbåge>// - **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([#24109](facebook/react#24109)) //<Luna>// - **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([#24107](facebook/react#24107)) //<salazarm>// - **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([#24026](facebook/react#24026)) //<Luna Ruan>// - **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([#23366](facebook/react#23366)) //<Luna>// - **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([#24081](facebook/react#24081)) //<Andrew Clark>// - **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>// - **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>// - **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>// - **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([#24068](facebook/react#24068)) //<Josh Story>// - **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([#24047](facebook/react#24047)) //<Sebastian Markbåge>// - **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([#24055](facebook/react#24055)) //<Sebastian Markbåge>// - **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([#24054](facebook/react#24054)) //<Sebastian Markbåge>// - **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([#23244](facebook/react#23244)) //<salazarm>// - **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([#24038](facebook/react#24038)) //<Sebastian Markbåge>// - **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([#24037](facebook/react#24037)) //<Sebastian Markbåge>// - **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([#24034](facebook/react#24034)) //<Josh Story>// - **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([#24030](facebook/react#24030)) //<Sebastian Markbåge>// - **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([#24025](facebook/react#24025)) //<Sebastian Markbåge>// - **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([#24024](facebook/react#24024)) //<salazarm>// - **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([#23386](facebook/react#23386)) //<Joshua Gross>// - **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([#23195](facebook/react#23195)) //<BIKI DAS>// - **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([#23393](facebook/react#23393)) //<Luna Ruan>// Changelog: [General][Changed] - React Native sync for revisions 1780659...1159ff6 jest_e2e[run_all_tests] Reviewed By: lunaleaps Differential Revision: D34928167 fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb
zhengjitf
pushed a commit
to zhengjitf/react
that referenced
this pull request
Apr 15, 2022
…nt render (facebook#24082) * Pass children to hydration root constructor I already made this change for the concurrent root API in facebook#23309. This does the same thing for the legacy API. Doesn't change any behavior, but I will use this in the next steps. * Add isRootDehydrated function Currently this does nothing except read a boolean field, but I'm about to change this logic. Since this is accessed by React DOM, too, I put the function in a separate module that can be deep imported. Previously, it was accessing the FiberRoot directly. The reason it's a separate module is to break a circular dependency between React DOM and the reconciler. * Allow updates at lower pri without forcing client render Currently, if a root is updated before the shell has finished hydrating (for example, due to a top-level navigation), we immediately revert to client rendering. This is rare because the root is expected is finish quickly, but not exceedingly rare because the root may be suspended. This adds support for updating the root without forcing a client render as long as the update has lower priority than the initial hydration, i.e. if the update is wrapped in startTransition. To implement this, I had to do some refactoring. The main idea here is to make it closer to how we implement hydration in Suspense boundaries: - I moved isDehydrated from the shared FiberRoot object to the HostRoot's state object. - In the begin phase, I check if the root has received an by comparing the new children to the initial children. If they are different, we revert to client rendering, and set isDehydrated to false using a derived state update (a la getDerivedStateFromProps). - There are a few places where we used to set root.isDehydrated to false as a way to force a client render. Instead, I set the ForceClientRender flag on the root work-in-progress fiber. - Whenever we fall back to client rendering, I log a recoverable error. The overall code structure is almost identical to the corresponding logic for Suspense components. The reason this works is because if the update has lower priority than the initial hydration, it won't be processed during the hydration render, so the children will be the same. We can go even further and allow updates at _higher_ priority (though not sync) by implementing selective hydration at the root, like we do for Suspense boundaries: interrupt the current render, attempt hydration at slightly higher priority than the update, then continue rendering the update. I haven't implemented this yet, but I've structured the code in anticipation of adding this later. * Wrap useMutableSource logic in feature flag
Saadnajmi
pushed a commit
to Saadnajmi/react-native-macos
that referenced
this pull request
Jan 15, 2023
Summary: This sync includes the following changes: - **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([facebook#24088](facebook/react#24088)) //<Sebastian Silbermann>// - **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([facebook#24131](facebook/react#24131)) //<David McCabe>// - **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([facebook#24082](facebook/react#24082)) //<Andrew Clark>// - **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([facebook#24126](facebook/react#24126)) //<Sebastian Markbåge>// - **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([facebook#24109](facebook/react#24109)) //<Luna>// - **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([facebook#24107](facebook/react#24107)) //<salazarm>// - **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([facebook#24026](facebook/react#24026)) //<Luna Ruan>// - **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([facebook#23366](facebook/react#23366)) //<Luna>// - **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([facebook#24081](facebook/react#24081)) //<Andrew Clark>// - **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>// - **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>// - **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>// - **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([facebook#24068](facebook/react#24068)) //<Josh Story>// - **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([facebook#24047](facebook/react#24047)) //<Sebastian Markbåge>// - **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([facebook#24055](facebook/react#24055)) //<Sebastian Markbåge>// - **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([facebook#24054](facebook/react#24054)) //<Sebastian Markbåge>// - **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([facebook#23244](facebook/react#23244)) //<salazarm>// - **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([facebook#24038](facebook/react#24038)) //<Sebastian Markbåge>// - **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([facebook#24037](facebook/react#24037)) //<Sebastian Markbåge>// - **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([facebook#24034](facebook/react#24034)) //<Josh Story>// - **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([facebook#24030](facebook/react#24030)) //<Sebastian Markbåge>// - **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([facebook#24025](facebook/react#24025)) //<Sebastian Markbåge>// - **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([facebook#24024](facebook/react#24024)) //<salazarm>// - **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([facebook#23386](facebook/react#23386)) //<Joshua Gross>// - **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([facebook#23195](facebook/react#23195)) //<BIKI DAS>// - **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([facebook#23393](facebook/react#23393)) //<Luna Ruan>// Changelog: [General][Changed] - React Native sync for revisions 1780659...1159ff6 jest_e2e[run_all_tests] Reviewed By: lunaleaps Differential Revision: D34928167 fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, if a root is updated before the shell has finished hydrating (for example, due to a top-level navigation), we immediately revert to client rendering. This is rare because the root is expected is finish quickly, but not exceedingly rare because the root may be suspended.
This adds support for updating the root without forcing a client render as long as the update has lower priority than the initial hydration, i.e. if the update is wrapped in startTransition.
To implement this, I had to do some refactoring. The main idea here is to make it closer to how we implement hydration in Suspense boundaries:
The overall code structure is almost identical to the corresponding logic for Suspense components.
The reason this works is because if the update has lower priority than the initial hydration, it won't be processed during the hydration render, so the children will be the same.
We can go even further and allow updates at higher priority (though not sync) by implementing selective hydration at the root, like we do for Suspense boundaries: interrupt the current render, attempt hydration at slightly higher priority than the update, then continue rendering the update. I haven't implemented this yet, but I've structured the code in anticipation of adding this later.