Skip to content
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

Allow suspending in the shell during hydration #23304

Merged
merged 2 commits into from
Feb 16, 2022

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Feb 15, 2022

Builds on behavior added in #23267.

Initial hydration should be allowed to suspend in the shell. In practice, this happens because the code for the outer shell hasn't loaded yet.

Currently if you try to do this, it errors because it expects there to be a parent Suspense boundary, because without a fallback we can't produce a consistent tree. However, for non-sync updates, we don't need to produce a consistent tree immediately — we can delay the commit until the data resolves.

In #23267, I added support for suspending without a parent boundary if the update was wrapped with startTransition. Here, I've expanded this to include hydration, too.

I wonder if we should expand this even further to include all non-sync/discrete updates. Updated to include all non-sync/discrete updates.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 15, 2022
const jsdom = new JSDOM(
'<!DOCTYPE html><html><head></head><body><div id="container">',
{
runScripts: 'dangerously',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL

@sizebot
Copy link

sizebot commented Feb 15, 2022

Comparing: 27b5699...58e6771

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 130.59 kB 130.59 kB = 41.86 kB 41.85 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 135.57 kB 135.57 kB = 43.34 kB 43.33 kB
facebook-www/ReactDOM-prod.classic.js = 431.64 kB 431.62 kB = 79.20 kB 79.20 kB
facebook-www/ReactDOM-prod.modern.js = 421.52 kB 421.50 kB = 77.73 kB 77.73 kB
facebook-www/ReactDOMForked-prod.classic.js = 431.64 kB 431.62 kB = 79.21 kB 79.21 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 58e6771

@acdlite acdlite force-pushed the allow-suspending-in-the-shell branch from ad66295 to 863fa64 Compare February 15, 2022 22:01
@@ -389,17 +389,6 @@ describe('ReactSuspense', () => {
expect(root).toMatchRenderedOutput('Hi');
});

it('throws if tree suspends and none of the Suspense ancestors have a boundary', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted this test because it was a duplicate of the one in ReactSuspenseWithNoopRenderer

Builds on behavior added in facebook#23267.

Initial hydration should be allowed to suspend in the shell. In
practice, this happens because the code for the outer shell hasn't
loaded yet.

Currently if you try to do this, it errors because it expects there to
be a parent Suspense boundary, because without a fallback we can't
produce a consistent tree. However, for non-sync updates, we don't need
to produce a consistent tree immediately — we can delay the commit
until the data resolves.

In facebook#23267, I added support for suspending without a parent boundary if
the update was wrapped with `startTransition`. Here, I've expanded this
to include hydration, too.

I wonder if we should expand this even further to include all non-sync/
discrete updates.
Instead of erroring, we can delay the commit.

The only time we'll continue to error when there's no parent Suspense
boundary is during sync/discrete updates, because those are expected to
produce a complete tree synchronously to maintain consistency with
external state.
@Rk78678

This comment was marked as spam.

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
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
* Allow suspending in the shell during hydration

Builds on behavior added in facebook#23267.

Initial hydration should be allowed to suspend in the shell. In
practice, this happens because the code for the outer shell hasn't
loaded yet.

Currently if you try to do this, it errors because it expects there to
be a parent Suspense boundary, because without a fallback we can't
produce a consistent tree. However, for non-sync updates, we don't need
to produce a consistent tree immediately — we can delay the commit
until the data resolves.

In facebook#23267, I added support for suspending without a parent boundary if
the update was wrapped with `startTransition`. Here, I've expanded this
to include hydration, too.

I wonder if we should expand this even further to include all non-sync/
discrete updates.

* Allow suspending in shell for all non-sync updates

Instead of erroring, we can delay the commit.

The only time we'll continue to error when there's no parent Suspense
boundary is during sync/discrete updates, because those are expected to
produce a complete tree synchronously to maintain consistency with
external state.
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
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants