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 outside a Suspense boundary #23267

Merged
merged 1 commit into from
Feb 11, 2022

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Feb 10, 2022

(If the update is wrapped in startTransition)

Currently you're not allowed to suspend outside of a Suspense boundary. We throw an error:

A React component suspended while rendering, but no fallback UI was specified

We treat this case like an error because discrete renders are expected to finish synchronously to maintain consistency with external state. However, during a concurrent transition (startTransition), what we can do instead is treat this case like a refresh transition: suspend the commit without showing a fallback.

The behavior is roughly as if there were a built-in Suspense boundary at the root of the app with unstable_avoidThisFallback enabled. Conceptually it's very similar because during hydration you're already showing server-rendered UI; there's no need to replace that with a fallback when something suspends.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 10, 2022
@sizebot
Copy link

sizebot commented Feb 10, 2022

Comparing: e0af1aa...c14ca1f

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 +0.11% 130.35 kB 130.49 kB +0.02% 41.82 kB 41.83 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.10% 135.53 kB 135.67 kB +0.10% 43.35 kB 43.39 kB
facebook-www/ReactDOM-prod.classic.js +0.18% 431.17 kB 431.92 kB +0.13% 79.11 kB 79.22 kB
facebook-www/ReactDOM-prod.modern.js +0.18% 421.11 kB 421.87 kB +0.14% 77.68 kB 77.79 kB
facebook-www/ReactDOMForked-prod.classic.js +0.18% 431.17 kB 431.92 kB +0.14% 79.11 kB 79.22 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.development.js +0.38% 614.44 kB 616.80 kB +0.43% 134.94 kB 135.53 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.development.js +0.38% 614.44 kB 616.80 kB +0.43% 134.94 kB 135.53 kB
oss-stable-semver/react-test-renderer/umd/react-test-renderer.development.js +0.38% 644.16 kB 646.61 kB +0.41% 136.43 kB 136.99 kB
oss-stable/react-test-renderer/umd/react-test-renderer.development.js +0.38% 644.16 kB 646.61 kB +0.41% 136.43 kB 136.99 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-dev.js +0.37% 627.00 kB 629.31 kB +0.44% 136.12 kB 136.71 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.development.js +0.37% 641.12 kB 643.48 kB +0.42% 140.62 kB 141.21 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.development.js +0.37% 672.16 kB 674.61 kB +0.40% 142.12 kB 142.68 kB
oss-stable-semver/react-art/cjs/react-art.development.js +0.35% 669.69 kB 672.04 kB +0.41% 144.78 kB 145.38 kB
oss-stable/react-art/cjs/react-art.development.js +0.35% 669.69 kB 672.04 kB +0.41% 144.78 kB 145.38 kB
facebook-www/ReactTestRenderer-dev.classic.js +0.34% 675.15 kB 677.46 kB +0.41% 145.73 kB 146.32 kB
facebook-www/ReactTestRenderer-dev.modern.js +0.34% 675.16 kB 677.48 kB +0.41% 145.74 kB 146.33 kB
oss-experimental/react-art/cjs/react-art.development.js +0.34% 696.40 kB 698.75 kB +0.39% 150.43 kB 151.01 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.development.js +0.32% 730.23 kB 732.58 kB +0.37% 155.49 kB 156.07 kB
oss-stable/react-reconciler/cjs/react-reconciler.development.js +0.32% 730.23 kB 732.58 kB +0.37% 155.49 kB 156.07 kB
react-native/implementations/ReactFabric-dev.js +0.32% 723.94 kB 726.24 kB +0.39% 157.75 kB 158.37 kB
oss-stable-semver/react-art/umd/react-art.development.js +0.32% 772.88 kB 775.32 kB +0.35% 163.07 kB 163.63 kB
oss-stable/react-art/umd/react-art.development.js +0.32% 772.88 kB 775.32 kB +0.35% 163.07 kB 163.63 kB
react-native/implementations/ReactNativeRenderer-dev.js +0.31% 736.36 kB 738.66 kB +0.38% 160.48 kB 161.08 kB
oss-experimental/react-reconciler/cjs/react-reconciler.development.js +0.31% 757.13 kB 759.48 kB +0.37% 161.17 kB 161.77 kB
facebook-www/ReactART-dev.modern.js +0.31% 749.69 kB 752.00 kB +0.37% 160.10 kB 160.69 kB
oss-experimental/react-art/umd/react-art.development.js +0.30% 800.89 kB 803.33 kB +0.33% 168.68 kB 169.23 kB
facebook-www/ReactART-dev.classic.js +0.30% 759.91 kB 762.21 kB +0.37% 162.22 kB 162.81 kB
react-native/implementations/ReactFabric-dev.fb.js +0.30% 766.20 kB 768.50 kB +0.36% 165.48 kB 166.07 kB
react-native/implementations/ReactNativeRenderer-dev.fb.js +0.30% 776.76 kB 779.06 kB +0.34% 167.81 kB 168.39 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.development.js +0.24% 991.25 kB 993.61 kB +0.25% 224.67 kB 225.24 kB
oss-stable-semver/react-dom/cjs/react-dom.development.js +0.23% 1,002.16 kB 1,004.51 kB +0.24% 225.10 kB 225.64 kB
oss-stable/react-dom/cjs/react-dom.development.js +0.23% 1,002.16 kB 1,004.51 kB +0.24% 225.10 kB 225.64 kB
oss-stable-semver/react-dom/umd/react-dom.development.js +0.23% 1,051.89 kB 1,054.33 kB +0.24% 227.56 kB 228.11 kB
oss-stable/react-dom/umd/react-dom.development.js +0.23% 1,051.89 kB 1,054.33 kB +0.24% 227.56 kB 228.11 kB
facebook-www/ReactDOMTesting-dev.modern.js +0.23% 1,013.61 kB 1,015.93 kB +0.26% 228.52 kB 229.12 kB
oss-experimental/react-dom/cjs/react-dom.development.js +0.23% 1,031.51 kB 1,033.86 kB +0.23% 231.22 kB 231.76 kB
oss-experimental/react-dom/umd/react-dom.development.js +0.23% 1,082.66 kB 1,085.10 kB +0.24% 233.80 kB 234.36 kB
facebook-www/ReactDOMTesting-dev.classic.js +0.22% 1,040.42 kB 1,042.74 kB +0.26% 233.91 kB 234.52 kB
facebook-www/ReactDOMForked-dev.modern.js +0.21% 1,106.85 kB 1,109.16 kB +0.24% 245.03 kB 245.61 kB
facebook-www/ReactDOM-dev.modern.js +0.21% 1,106.85 kB 1,109.16 kB +0.24% 245.04 kB 245.62 kB
facebook-www/ReactDOMForked-dev.classic.js +0.20% 1,129.03 kB 1,131.33 kB +0.23% 249.19 kB 249.77 kB
facebook-www/ReactDOM-dev.classic.js +0.20% 1,129.03 kB 1,131.33 kB +0.23% 249.19 kB 249.77 kB

Generated by 🚫 dangerJS against c14ca1f

@acdlite acdlite force-pushed the suspend-in-shell branch 2 times, most recently from f00c02d to b7029ce Compare February 10, 2022 08:13
@acdlite acdlite marked this pull request as ready for review February 10, 2022 08:24
(If the update is wrapped in startTransition)

Currently you're not allowed to suspend outside of a Suspense boundary.
We throw an error:

> A React component suspended while rendering, but no fallback UI
was specified

We treat this case like an error because discrete renders are expected
to finish synchronously to maintain consistency with external state.
However, during a concurrent transition (startTransition), what we can
do instead is treat this case like a refresh transition: suspend the
commit without showing a fallback.

The behavior is roughly as if there were a built-in Suspense boundary
at the root of the app with unstable_avoidThisFallback enabled.
Conceptually it's very similar because during hydration you're already
showing server-rendered UI; there's no need to replace that with
a fallback when something suspends.
Comment on lines +487 to +493
// This is a transition. Suspend. Since we're not activating a Suspense
// boundary, this will unwind all the way to the root without performing
// a second pass to render a fallback. (This is arguably how refresh
// transitions should work, too, since we're not going to commit the
// fallbacks anyway.)
attachPingListener(root, wakeable, rootRenderLanes);
renderDidSuspendDelayIfPossible();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the key change. Because we exit throwException without marking a boundary, the root will unwind all the way to the root without completing.

exitStatus = recoverFromConcurrentError(root, errorRetryLanes);
// We assume the tree is now consistent because we didn't yield to any
// concurrent events.
if (exitStatus === RootDidNotComplete) {
Copy link
Collaborator Author

@acdlite acdlite Feb 10, 2022

Choose a reason for hiding this comment

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

This is a new root exit status for when the render phase exits without completing. We treat this like a refresh transition: skip the commit phase and suspend the root so we don't continue working on it.

In the future, I expect we could use this mechanism to unwind from an aborted render without doing a second pass to render fallbacks and error boundaries.

@gaearon
Copy link
Collaborator

gaearon commented Feb 10, 2022

Do you have a concern that this makes it easier to accidentally miss a case where you don't have a boundary (since it doesn't error right away) and then someone makes it synchronous and wouldn't notice the error?

@acdlite
Copy link
Collaborator Author

acdlite commented Feb 10, 2022

Do you have a concern that this makes it easier to accidentally miss a case where you don't have a boundary (since it doesn't error right away) and then someone makes it synchronous and wouldn't notice the error?

Yeah that's possible. There are legit reasons to not have a Suspense boundary though, like suspending in the "shell" of the app during hydration — stuff in the document head that doesn't have an appropriate fallback. So this change is about supporting those.

Longer term, what we could do instead of throwing is make the root act more like unstable_avoidThisFallback with a null placeholder. We hide everything offscreen then toggle it back when the data resolves. Then the root becomes a special case of regular avoided fallbacks. In all such cases, we will log with onRecoverableError.

The reason we throw currently is because of things like flushSync where you're expected to be able to read the finished result synchronously. But maybe we can throw only in the case of flushSync and do the unstable_avoidThisFallback thing for everything else.

@acdlite
Copy link
Collaborator Author

acdlite commented Feb 11, 2022

We should change the error message to recommend startTransition. I'll do this in a follow up.

@acdlite acdlite merged commit 796fff5 into facebook:main Feb 11, 2022
acdlite added a commit to acdlite/react that referenced this pull request Feb 15, 2022
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.
acdlite added a commit to acdlite/react that referenced this pull request Feb 15, 2022
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.
acdlite added a commit to acdlite/react that referenced this pull request Feb 16, 2022
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.
acdlite added a commit that referenced this pull request Feb 16, 2022
* Allow suspending in the shell during hydration

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.

* 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.
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Feb 17, 2022
Summary:
This sync includes the following changes:
- **[27b569969](facebook/react@27b569969 )**: Simplify cache pool contexts ([#23280](facebook/react#23280)) //<Andrew Clark>//
- **[1fb0d0687](facebook/react@1fb0d0687 )**: [Devtools][Transition Tracing] Add Transition callbacks to createRoot ([#23276](facebook/react#23276)) //<Luna Ruan>//
- **[a6987bee7](facebook/react@a6987bee7 )**: add <TracingMarker> component boilerplate ([#23275](facebook/react#23275)) //<Luna Ruan>//
- **[796fff548](facebook/react@796fff548 )**: Allow suspending outside a Suspense boundary ([#23267](facebook/react#23267)) //<Andrew Clark>//
- **[64223fed8](facebook/react@64223fed8 )**: Fix: Multiple hydration errors in same render ([#23273](facebook/react#23273)) //<Andrew Clark>//
- **[efd8f6442](facebook/react@efd8f6442 )**: Resolve default onRecoverableError at root init ([#23264](facebook/react#23264)) //<Andrew Clark>//
- **[e0af1aabe](facebook/react@e0af1aabe )**: Fix wrong context argument to `apply` //<Andrew Clark>//
- **[9b5e0517b](facebook/react@9b5e0517b )**: Remove deprecated wildcard folder mapping ([#23256](facebook/react#23256)) //<Andrew Clark>//
- **[274b9fb16](facebook/react@274b9fb16 )**: Remove path resolution from internal forks plugin ([#23255](facebook/react#23255)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions a3bde79...27b5699

jest_e2e[run_all_tests]

Reviewed By: rickhanlonii, kacieb

Differential Revision: D34241986

fbshipit-source-id: f6ab62df2a918728864283b4f13201275eb3b8a3
@gaearon gaearon mentioned this pull request Mar 29, 2022
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
(If the update is wrapped in startTransition)

Currently you're not allowed to suspend outside of a Suspense boundary.
We throw an error:

> A React component suspended while rendering, but no fallback UI
was specified

We treat this case like an error because discrete renders are expected
to finish synchronously to maintain consistency with external state.
However, during a concurrent transition (startTransition), what we can
do instead is treat this case like a refresh transition: suspend the
commit without showing a fallback.

The behavior is roughly as if there were a built-in Suspense boundary
at the root of the app with unstable_avoidThisFallback enabled.
Conceptually it's very similar because during hydration you're already
showing server-rendered UI; there's no need to replace that with
a fallback when something suspends.
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.
//
// This should only happen during a concurrent render, not a discrete or
// synchronous update. We should have already checked for this when we
// unwound the stack.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just noting here for reference when we follow on the task. This doesn't happen atm. At least in a synchronous forced hydration if something suspends, we can end up with this status and subsequently throw.

@@ -1149,6 +1163,10 @@ function performSyncWorkOnRoot(root) {
throw fatalError;
}

if (exitStatus === RootDidNotComplete) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We somehow get here for force sync hydration.

Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
Summary:
This sync includes the following changes:
- **[27b569969](facebook/react@27b569969 )**: Simplify cache pool contexts ([facebook#23280](facebook/react#23280)) //<Andrew Clark>//
- **[1fb0d0687](facebook/react@1fb0d0687 )**: [Devtools][Transition Tracing] Add Transition callbacks to createRoot ([facebook#23276](facebook/react#23276)) //<Luna Ruan>//
- **[a6987bee7](facebook/react@a6987bee7 )**: add <TracingMarker> component boilerplate ([facebook#23275](facebook/react#23275)) //<Luna Ruan>//
- **[796fff548](facebook/react@796fff548 )**: Allow suspending outside a Suspense boundary ([facebook#23267](facebook/react#23267)) //<Andrew Clark>//
- **[64223fed8](facebook/react@64223fed8 )**: Fix: Multiple hydration errors in same render ([facebook#23273](facebook/react#23273)) //<Andrew Clark>//
- **[efd8f6442](facebook/react@efd8f6442 )**: Resolve default onRecoverableError at root init ([facebook#23264](facebook/react#23264)) //<Andrew Clark>//
- **[e0af1aabe](facebook/react@e0af1aabe )**: Fix wrong context argument to `apply` //<Andrew Clark>//
- **[9b5e0517b](facebook/react@9b5e0517b )**: Remove deprecated wildcard folder mapping ([facebook#23256](facebook/react#23256)) //<Andrew Clark>//
- **[274b9fb16](facebook/react@274b9fb16 )**: Remove path resolution from internal forks plugin ([facebook#23255](facebook/react#23255)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions a3bde79...27b5699

jest_e2e[run_all_tests]

Reviewed By: rickhanlonii, kacieb

Differential Revision: D34241986

fbshipit-source-id: f6ab62df2a918728864283b4f13201275eb3b8a3
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.

6 participants