[Fizz] Support SuspenseList revealOrder="together" #33311
Merged
Conversation
This file contains hidden or 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
5 tasks
f5373e4 to
0eaab16
Compare
sebmarkbage
commented
May 20, 2025
| @@ -194,6 +193,10 @@ export function getViewTransitionFormatContext( | |||
| return parentContext; | |||
| } | |||
|
|
|||
| export function canHavePreamble(): boolean { | |||
| return false; | |||
There was a problem hiding this comment.
@gnoff Top level Suspense creates a scenario where you sometimes have to emit some kind of top level HTML tag to force Suspense boundaries to flush early. This shouldn't be necessary in renderToString which is a legacy API.
I think I can basically disable the feature this way.
dfe8b08 to
f977259
Compare
sebmarkbage
commented
May 20, 2025
| if (allComplete) { | ||
| unblockSuspenseListRow(request, row); | ||
| } | ||
| } |
There was a problem hiding this comment.
This optimization wouldn't be safe if this had some hoistable state because it would not have been hoisted as part of hoistPreambleState since these boundaries weren't COMPLETE by the time we started flushing the shell.
Not sure how to solve that.
There was a problem hiding this comment.
Probably have to find a way to move this optimization earlier.
There was a problem hiding this comment.
Moved to render phase. 1c50823
gnoff
approved these changes
May 20, 2025
eps1lon
approved these changes
May 20, 2025
f977259 to
1c50823
Compare
sebmarkbage
added a commit
that referenced
this pull request
May 20, 2025
…future rows (#33312) Stacked on #33311. When a row contains Suspense boundaries that themselves depend on CSS, they will not resolve until the CSS has loaded on the client. We need future rows in a list to be blocked until this happens. We could do something in the runtime but a simpler approach is to just add those CSS dependencies to all those boundaries as well. To do this, we first hoist the HoistableState from a completed boundary onto its parent row. Then when the row finishes do we hoist it onto the next row and onto any boundaries within that row.
github-actions bot
pushed a commit
that referenced
this pull request
May 20, 2025
Stacked on #33308. For "together" mode, we can be a self-blocking row that adds all its boundaries to the blocked set, but there's no parent row that unblocks it. A particular quirk of this mode is that it's not enough to just unblock them all on the server together. Because if one boundary downloads all its html and then issues a complete instruction it'll appear before the others while streaming in. What we actually want is to reveal them all in a single batch. This implementation takes a short cut by unblocking the rows in `flushPartialBoundary`. That ensures that all the segments of every boundary has a chance to flush before we start emitting any of the complete boundary instructions. Once the last one unblocks, all the complete boundary instructions are queued. Ideally this would be a single `<script>` tag so that they can't be split up even if we get a chunk containing some of them. ~A downside of this approach is that we always outline these boundaries. We could inline them if they all complete before the parent flushes. E.g. by checking if the row is blocked only by its own boundaries and if all the boundaries would fit without getting outlined, then we can inline them all at once.~ I went ahead and did this because it solves an issue with `renderToString` where it doesn't support the script runtime so it can only handle this if inlined. DiffTrain build for [99aa685](99aa685)
github-actions bot
pushed a commit
that referenced
this pull request
May 20, 2025
…future rows (#33312) Stacked on #33311. When a row contains Suspense boundaries that themselves depend on CSS, they will not resolve until the CSS has loaded on the client. We need future rows in a list to be blocked until this happens. We could do something in the runtime but a simpler approach is to just add those CSS dependencies to all those boundaries as well. To do this, we first hoist the HoistableState from a completed boundary onto its parent row. Then when the row finishes do we hoist it onto the next row and onto any boundaries within that row. DiffTrain build for [50389e1](50389e1)
17 tasks
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.
Stacked on #33308.
For "together" mode, we can be a self-blocking row that adds all its boundaries to the blocked set, but there's no parent row that unblocks it.
A particular quirk of this mode is that it's not enough to just unblock them all on the server together. Because if one boundary downloads all its html and then issues a complete instruction it'll appear before the others while streaming in. What we actually want is to reveal them all in a single batch.
This implementation takes a short cut by unblocking the rows in
flushPartialBoundary. That ensures that all the segments of every boundary has a chance to flush before we start emitting any of the complete boundary instructions. Once the last one unblocks, all the complete boundary instructions are queued. Ideally this would be a single<script>tag so that they can't be split up even if we get a chunk containing some of them.A downside of this approach is that we always outline these boundaries. We could inline them if they all complete before the parent flushes. E.g. by checking if the row is blocked only by its own boundaries and if all the boundaries would fit without getting outlined, then we can inline them all at once.I went ahead and did this because it solves an issue withrenderToStringwhere it doesn't support the script runtime so it can only handle this if inlined.