Skip to content

Commit

Permalink
[Fizz] Don't flush empty segments (#24054)
Browse files Browse the repository at this point in the history
Before this change, we would sometimes write segments without any content
in them. For example for a Suspense boundary that immediately suspends
we might emit something like:

<div hidden id="123">
  <template id="456"></template>
</div>

Where the outer div is just a temporary wrapper and the inner one is a
placeholder for something to be added later.

This serves no purpose.

We should ideally have a heuristic that holds back segments based on byte
size and time. However, this is a straight forward clear win for now.
  • Loading branch information
sebmarkbage committed Mar 9, 2022
1 parent 11c5bb6 commit c91892e
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 3 deletions.
4 changes: 4 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,10 @@ describe('ReactDOMFizzServer', () => {
pipe(writable);
});
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);
// Because there is no content inside the Suspense boundary that could've
// been written, we expect to not see any additional partial data flushed
// yet.
expect(container.firstChild.nextSibling).toBe(null);
await act(async () => {
resolveElement({default: <Text text="Hello" />});
});
Expand Down
29 changes: 26 additions & 3 deletions packages/react-server/src/ReactFizzServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ function renderSuspenseBoundary(
// We use the safe form because we don't handle suspending here. Only error handling.
renderNode(request, task, content);
contentRootSegment.status = COMPLETED;
newBoundary.completedSegments.push(contentRootSegment);
queueCompletedSegment(newBoundary, contentRootSegment);
if (newBoundary.pendingTasks === 0) {
// This must have been the last segment we were waiting on. This boundary is now complete.
// Therefore we won't need the fallback. We early return so that we don't have to create
Expand Down Expand Up @@ -1430,6 +1430,29 @@ function abortTask(task: Task): void {
}
}

function queueCompletedSegment(
boundary: SuspenseBoundary,
segment: Segment,
): void {
if (
segment.chunks.length === 0 &&
segment.children.length === 1 &&
segment.children[0].boundary === null
) {
// This is an empty segment. There's nothing to write, so we can instead transfer the ID
// to the child. That way any existing references point to the child.
const childSegment = segment.children[0];
childSegment.id = segment.id;
childSegment.parentFlushed = true;
if (childSegment.status === COMPLETED) {
queueCompletedSegment(boundary, childSegment);
}
} else {
const completedSegments = boundary.completedSegments;
completedSegments.push(segment);
}
}

function finishedTask(
request: Request,
boundary: Root | SuspenseBoundary,
Expand Down Expand Up @@ -1463,7 +1486,7 @@ function finishedTask(
// If it is a segment that was aborted, we'll write other content instead so we don't need
// to emit it.
if (segment.status === COMPLETED) {
boundary.completedSegments.push(segment);
queueCompletedSegment(boundary, segment);
}
}
if (boundary.parentFlushed) {
Expand All @@ -1482,8 +1505,8 @@ function finishedTask(
// If it is a segment that was aborted, we'll write other content instead so we don't need
// to emit it.
if (segment.status === COMPLETED) {
queueCompletedSegment(boundary, segment);
const completedSegments = boundary.completedSegments;
completedSegments.push(segment);
if (completedSegments.length === 1) {
// This is the first time since we last flushed that we completed anything.
// We can schedule this boundary to emit its partially completed segments early
Expand Down

0 comments on commit c91892e

Please sign in to comment.