Make prerendering always non-blocking with fix #31452
Merged
+428
−129
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
When a synchronous update suspends, and we prerender the siblings, the prerendering should be non-blocking so that we can immediately restart once the data arrives. This happens automatically when there's a Suspense boundary, because we immediately commit the boundary and then proceed to a Retry render, which are always concurrent. When there's not a Suspense boundary, there is no Retry, so we need to take care to switch from the synchronous work loop to the concurrent one, to enable time slicing.
We are getting infinite sync render loops from useSyncExternalStore render phase updates (Recoil). renderWasConcurrent within the do block made the sync pass pin the exitStatus to RootSuspended.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
josephsavona
reviewed
Nov 8, 2024
Comment on lines
+916
to
+917
| let renderWasConcurrent = shouldTimeSlice; | ||
|
|
There was a problem hiding this comment.
facepalm, could happen to anyone but damn this was hard to find
josephsavona
approved these changes
Nov 8, 2024
There was a problem hiding this comment.
DUDE. Amazing job debugging this, @jackpope and @rickhanlonii! Since the main code was already reviewed originally let's go ahead and land.
Just a couple comments on the test case, looks like some bits are maybe unused/unnecessary for the repro.
| } | ||
| } | ||
|
|
||
| function useBug<TValue>(path: string): SRConfigPathState<TValue> { |
github-actions bot
pushed a commit
that referenced
this pull request
Nov 8, 2024
We've previously failed to land this change due to some internal apps seeing infinite render loops due to external store state updates during render. It turns out that since the `renderWasConcurrent` var was moved into the do block, the sync render triggered from the external store check was stuck with a `RootSuspended` `exitStatus`. So this is not unique to sibling prerendering but more generally related to how we handle update to a sync external store during render. We've tested this build against local repros which now render without crashes. We will try to add a unit test to cover the scenario as well. --------- Co-authored-by: Andrew Clark <[email protected]> Co-authored-by: Rick Hanlon <[email protected]> DiffTrain build for [989af12](989af12)
github-actions bot
pushed a commit
that referenced
this pull request
Nov 8, 2024
We've previously failed to land this change due to some internal apps seeing infinite render loops due to external store state updates during render. It turns out that since the `renderWasConcurrent` var was moved into the do block, the sync render triggered from the external store check was stuck with a `RootSuspended` `exitStatus`. So this is not unique to sibling prerendering but more generally related to how we handle update to a sync external store during render. We've tested this build against local repros which now render without crashes. We will try to add a unit test to cover the scenario as well. --------- Co-authored-by: Andrew Clark <[email protected]> Co-authored-by: Rick Hanlon <[email protected]> DiffTrain build for [989af12](989af12)
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.
We've previously failed to land this change due to some internal apps seeing infinite render loops due to external store state updates during render. It turns out that since the
renderWasConcurrentvar was moved into the do block, the sync render triggered from the external store check was stuck with aRootSuspendedexitStatus. So this is not unique to sibling prerendering but more generally related to how we handle update to a sync external store during render.We've tested this build against local repros which now render without crashes. We will try to add a unit test to cover the scenario as well.