Switch to client rendering if root receives update #23309
Merged
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
the
React Core Team
acdlite
commented
Feb 16, 2022
| @@ -66,20 +66,21 @@ export function act(scope: () => Thenable<mixed> | void) { | |||
| // returned and 2) we could use async/await. Since it's only our used in | |||
| // our test suite, we should be able to. | |||
| try { | |||
| const thenable = scope(); | |||
| const result = scope(); | |||
There was a problem hiding this comment.
I updated our internal implementation of act to resolve to the return value of the scope function, like how the public implementation of act works. Not relevant to the rest of the PR, just wanted it for a test.
acdlite
changed the title
|
Comparing: f7f7ed0...3b122d5 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-update-during-hydration
branch
from
7e2f049 to
579dfbb
Compare
acdlite
commented
Feb 16, 2022
|
|
||
| // @gate !enableSyncDefaultUpdates | ||
| // @gate enableUseMutableSource | ||
| it('should detect a tear during a higher priority interruption', () => { |
There was a problem hiding this comment.
I deleted this test instead of rewriting it since useMutableSource is slated for removal anyway
acdlite
force-pushed
the
root-update-during-hydration
branch
from
579dfbb to
ca8f39b
Compare
salazarm
reviewed
Feb 16, 2022
| @@ -244,6 +245,8 @@ function findHostInstanceWithWarning( | |||
| export function createContainer( | |||
| containerInfo: Container, | |||
| tag: RootTag, | |||
| // TODO: We can remove hydration-specific stuff from createContainer once | |||
| // we delete legacy mode. The new root API uses createDehydratedContainer. | |||
There was a problem hiding this comment.
typo? You wrote createDehydratedContainer instead of createHydrationContainer ?
salazarm
approved these changes
Feb 16, 2022
If a hydration root receives an update before the outermost shell has finished hydrating, we should give up hydrating and switch to client rendering. Since the shell is expected to commit quickly, this doesn't happen that often. The most common sequence is something in the shell suspends, and then the user quickly navigates to a different screen, triggering a top-level update. Instead of immediately switching to client rendering, we could first attempt to hydration at higher priority, like we do for updates that occur inside nested dehydrated trees. But since this case is expected to be rare, and mainly only happens when the shell is suspended, an attempt at higher priority would likely end up suspending again anyway, so it would be wasted effort. Implementing it this way would also require us to add a new lane especially for root hydration. For simplicity's sake, we'll immediately switch to client rendering. In the future, if we find another use case for a root hydration lane, we'll reconsider.
acdlite
force-pushed
the
root-update-during-hydration
branch
from
ca8f39b to
3b122d5
Compare
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Feb 24, 2022
Summary: This sync includes the following changes: - **[4de99b3ca](facebook/react@4de99b3ca )**: fix getSnapshot warning when a selector returns NaN ([#23333](facebook/react#23333)) //<OGURA Daiki>// - **[40eaa22d9](facebook/react@40eaa22d9 )**: Remove dependency on Offscreen Fiber updateQueue for React Cache ([#23229](facebook/react#23229)) //<Luna Ruan>// - **[caf6d4707](facebook/react@caf6d4707 )**: Enable enableCache on Test Renderer native ([#23314](facebook/react#23314)) //<David McCabe>// - **[419ccc2b1](facebook/react@419ccc2b1 )**: Land skipUnmountedBoundaries experiment ([#23322](facebook/react#23322)) //<Andrew Clark>// - **[54f785bc5](facebook/react@54f785bc5 )**: Disallow comments as DOM containers for createRoot ([#23321](facebook/react#23321)) //<Andrew Clark>// - **[e9aa9592c](facebook/react@e9aa9592c )**: change ReactBatchConfig.transition //<Luna Ruan>// - **[51c8411d9](facebook/react@51c8411d9 )**: Log a recoverable error whenever hydration fails ([#23319](facebook/react#23319)) //<Andrew Clark>// - **[79ed5e18f](facebook/react@79ed5e18f )**: Delete vestigial RetryAfterError logic ([#23312](facebook/react#23312)) //<Andrew Clark>// - **[80059bb73](facebook/react@80059bb73 )**: Switch to client rendering if root receives update ([#23309](facebook/react#23309)) //<Andrew Clark>// - **[f7f7ed089](facebook/react@f7f7ed089 )**: Allow suspending in the shell during hydration ([#23304](facebook/react#23304)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 27b5699...4de99b3 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D34399162 fbshipit-source-id: 5c49e2bdcf63eb6a601cfa6a4e4b8f2e1f83e2dd
4 tasks
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Mar 12, 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.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Mar 12, 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.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Mar 12, 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.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Mar 20, 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.
acdlite
added a commit
that referenced
this pull request
Mar 20, 2022
…nt render (#24082) * Pass children to hydration root constructor I already made this change for the concurrent root API in #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
zhengjitf
pushed a commit
to zhengjitf/react
that referenced
this pull request
Apr 15, 2022
If a hydration root receives an update before the outermost shell has finished hydrating, we should give up hydrating and switch to client rendering. Since the shell is expected to commit quickly, this doesn't happen that often. The most common sequence is something in the shell suspends, and then the user quickly navigates to a different screen, triggering a top-level update. Instead of immediately switching to client rendering, we could first attempt to hydration at higher priority, like we do for updates that occur inside nested dehydrated trees. But since this case is expected to be rare, and mainly only happens when the shell is suspended, an attempt at higher priority would likely end up suspending again anyway, so it would be wasted effort. Implementing it this way would also require us to add a new lane especially for root hydration. For simplicity's sake, we'll immediately switch to client rendering. In the future, if we find another use case for a root hydration lane, we'll reconsider.
zhengjitf
pushed a commit
to zhengjitf/react
that referenced
this pull request
Apr 15, 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.
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: - **[4de99b3ca](facebook/react@4de99b3ca )**: fix getSnapshot warning when a selector returns NaN ([facebook#23333](facebook/react#23333)) //<OGURA Daiki>// - **[40eaa22d9](facebook/react@40eaa22d9 )**: Remove dependency on Offscreen Fiber updateQueue for React Cache ([facebook#23229](facebook/react#23229)) //<Luna Ruan>// - **[caf6d4707](facebook/react@caf6d4707 )**: Enable enableCache on Test Renderer native ([facebook#23314](facebook/react#23314)) //<David McCabe>// - **[419ccc2b1](facebook/react@419ccc2b1 )**: Land skipUnmountedBoundaries experiment ([facebook#23322](facebook/react#23322)) //<Andrew Clark>// - **[54f785bc5](facebook/react@54f785bc5 )**: Disallow comments as DOM containers for createRoot ([facebook#23321](facebook/react#23321)) //<Andrew Clark>// - **[e9aa9592c](facebook/react@e9aa9592c )**: change ReactBatchConfig.transition //<Luna Ruan>// - **[51c8411d9](facebook/react@51c8411d9 )**: Log a recoverable error whenever hydration fails ([facebook#23319](facebook/react#23319)) //<Andrew Clark>// - **[79ed5e18f](facebook/react@79ed5e18f )**: Delete vestigial RetryAfterError logic ([facebook#23312](facebook/react#23312)) //<Andrew Clark>// - **[80059bb73](facebook/react@80059bb73 )**: Switch to client rendering if root receives update ([facebook#23309](facebook/react#23309)) //<Andrew Clark>// - **[f7f7ed089](facebook/react@f7f7ed089 )**: Allow suspending in the shell during hydration ([facebook#23304](facebook/react#23304)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 27b5699...4de99b3 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D34399162 fbshipit-source-id: 5c49e2bdcf63eb6a601cfa6a4e4b8f2e1f83e2dd
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.
If a hydration root receives an update before the outermost shell has finished hydrating, we should give up hydrating and switch to client rendering.
Since the shell is expected to commit quickly, this doesn't happen that often. The most common sequence is something in the shell suspends, and then the user quickly navigates to a different screen, triggering a top-level update.
Instead of immediately switching to client rendering, we could first attempt to hydration at higher priority, like we do for updates that occur inside nested dehydrated trees.
But since this case is expected to be rare, and mainly only happens when the shell is suspended, an attempt at higher priority would likely end up suspending again anyway, so it would be wasted effort. Implementing it this way would also require us to add a new lane especially for root hydration. For simplicity's sake, we'll immediately switch to client rendering. In the future, if we find another use case for a root hydration lane, we'll reconsider.