[Fizz] Don't bail out of flushing if we still have pending root tasks #27385
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
The idea for this check is that we shouldn't flush anything before we flush the shell. That may or may not hold true in future formats like RN. It is a problem for resuming because with resuming it's possible to have root tasks that are used from resuming but the shell was already flushed so we can have completed boundaries before the shell has fully resumed. It's not technically necessary to bail early because there won't be anything in partialBoundaries or completedBoundaries because nothing gets added there unless the parent has already flushed. It's not exactly slow to have to check the length of three arrays so it's probably not a big deal. Flush partials in an early preamble needs further consideration regardless.
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: a5fc797...1fd724e Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
gnoff
approved these changes
Sep 18, 2023
sebmarkbage
added a commit
that referenced
this pull request
Sep 18, 2023
… replay errors or is aborted (#27386) Based on #27385. When we error or abort during replay, that doesn't actually error the component that errored because that has already rendered. The error only affects any child that is not yet completed. Therefore the error kind of gets thrown at the resumable point. The resumable point might be a hole in the replay path, in which case throwing there errors the parent boundary just the same as if the replay component errored. If the hole is inside a deeper Suspense boundary though, then it's that Suspense boundary that gets client rendered. I.e. the child boundary. We can still finish any siblings. In the shell all resumable points are inside a boundary since we must have finished the shell. Therefore if you error in the root, we just simply just turn all incomplete boundaries into client renders.
github-actions bot
pushed a commit
that referenced
this pull request
Sep 18, 2023
…#27385) The idea for this check is that we shouldn't flush anything before we flush the shell. That may or may not hold true in future formats like RN. It is a problem for resuming because with resuming it's possible to have root tasks that are used for resuming but the shell was already flushed so we can have completed boundaries before the shell has fully resumed. What matters is whether the parent has already flushed or not. It's not technically necessary to bail early because there won't be anything in partialBoundaries or completedBoundaries because nothing gets added there unless the parent has already flushed. It's not exactly slow to have to check the length of three arrays so it's probably not a big deal. Flush partials in an early preamble needs further consideration regardless. DiffTrain build for [df061b3](df061b3)
github-actions bot
pushed a commit
that referenced
this pull request
Sep 18, 2023
… replay errors or is aborted (#27386) Based on #27385. When we error or abort during replay, that doesn't actually error the component that errored because that has already rendered. The error only affects any child that is not yet completed. Therefore the error kind of gets thrown at the resumable point. The resumable point might be a hole in the replay path, in which case throwing there errors the parent boundary just the same as if the replay component errored. If the hole is inside a deeper Suspense boundary though, then it's that Suspense boundary that gets client rendered. I.e. the child boundary. We can still finish any siblings. In the shell all resumable points are inside a boundary since we must have finished the shell. Therefore if you error in the root, we just simply just turn all incomplete boundaries into client renders. DiffTrain build for [925c66a](925c66a)
kodiakhq bot
pushed a commit
to vercel/next.js
that referenced
this pull request
Sep 20, 2023
Update React from d6dcad6a8 to 2807d781a. ### React upstream changes - facebook/react#27387 - facebook/react#27386 - facebook/react#27385 - facebook/react#27379 - facebook/react#27382 Closes NEXT-1637
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…facebook#27385) The idea for this check is that we shouldn't flush anything before we flush the shell. That may or may not hold true in future formats like RN. It is a problem for resuming because with resuming it's possible to have root tasks that are used for resuming but the shell was already flushed so we can have completed boundaries before the shell has fully resumed. What matters is whether the parent has already flushed or not. It's not technically necessary to bail early because there won't be anything in partialBoundaries or completedBoundaries because nothing gets added there unless the parent has already flushed. It's not exactly slow to have to check the length of three arrays so it's probably not a big deal. Flush partials in an early preamble needs further consideration regardless.
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
… replay errors or is aborted (facebook#27386) Based on facebook#27385. When we error or abort during replay, that doesn't actually error the component that errored because that has already rendered. The error only affects any child that is not yet completed. Therefore the error kind of gets thrown at the resumable point. The resumable point might be a hole in the replay path, in which case throwing there errors the parent boundary just the same as if the replay component errored. If the hole is inside a deeper Suspense boundary though, then it's that Suspense boundary that gets client rendered. I.e. the child boundary. We can still finish any siblings. In the shell all resumable points are inside a boundary since we must have finished the shell. Therefore if you error in the root, we just simply just turn all incomplete boundaries into client renders.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
…#27385) The idea for this check is that we shouldn't flush anything before we flush the shell. That may or may not hold true in future formats like RN. It is a problem for resuming because with resuming it's possible to have root tasks that are used for resuming but the shell was already flushed so we can have completed boundaries before the shell has fully resumed. What matters is whether the parent has already flushed or not. It's not technically necessary to bail early because there won't be anything in partialBoundaries or completedBoundaries because nothing gets added there unless the parent has already flushed. It's not exactly slow to have to check the length of three arrays so it's probably not a big deal. Flush partials in an early preamble needs further consideration regardless. DiffTrain build for commit df061b3.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
… replay errors or is aborted (#27386) Based on #27385. When we error or abort during replay, that doesn't actually error the component that errored because that has already rendered. The error only affects any child that is not yet completed. Therefore the error kind of gets thrown at the resumable point. The resumable point might be a hole in the replay path, in which case throwing there errors the parent boundary just the same as if the replay component errored. If the hole is inside a deeper Suspense boundary though, then it's that Suspense boundary that gets client rendered. I.e. the child boundary. We can still finish any siblings. In the shell all resumable points are inside a boundary since we must have finished the shell. Therefore if you error in the root, we just simply just turn all incomplete boundaries into client renders. DiffTrain build for commit 925c66a.
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.
The idea for this check is that we shouldn't flush anything before we flush the shell. That may or may not hold true in future formats like RN.
It is a problem for resuming because with resuming it's possible to have root tasks that are used for resuming but the shell was already flushed so we can have completed boundaries before the shell has fully resumed. What matters is whether the parent has already flushed or not.
It's not technically necessary to bail early because there won't be anything in partialBoundaries or completedBoundaries because nothing gets added there unless the parent has already flushed.
It's not exactly slow to have to check the length of three arrays so it's probably not a big deal.
Flush partials in an early preamble needs further consideration regardless.