Allow suspending outside a Suspense boundary #23267
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
CLA Signed
React Core Team
|
Comparing: e0af1aa...c14ca1f 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
suspend-in-shell
branch
2 times, most recently
from
f00c02d to
b7029ce
Compare
(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.
acdlite
force-pushed
the
suspend-in-shell
branch
from
b7029ce to
c14ca1f
Compare
acdlite
commented
Feb 10, 2022
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(); |
There was a problem hiding this comment.
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.
acdlite
commented
Feb 10, 2022
| exitStatus = recoverFromConcurrentError(root, errorRetryLanes); | ||
| // We assume the tree is now consistent because we didn't yield to any | ||
| // concurrent events. | ||
| if (exitStatus === RootDidNotComplete) { |
There was a problem hiding this comment.
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.
salazarm
approved these changes
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 The reason we throw currently is because of things like |
|
We should change the error message to recommend startTransition. I'll do this in a follow up. |
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.
1 task
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
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.
sebmarkbage
reviewed
Jun 23, 2022
| // | ||
| // 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. |
There was a problem hiding this comment.
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) { | |||
There was a problem hiding this comment.
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
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 the update is wrapped in startTransition)
Currently you're not allowed to suspend outside of a Suspense boundary. We throw an error:
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.