From 142382e4505172faa9116850a31e4383c92f0123 Mon Sep 17 00:00:00 2001
From: Sebastian Markbage <sebastian@calyptus.eu>
Date: Mon, 7 Mar 2022 22:36:28 -0500
Subject: [PATCH] [Fizz] Don't flush empty segments

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.
---
 .../src/__tests__/ReactDOMFizzServer-test.js  |  4 ++
 packages/react-server/src/ReactFizzServer.js  | 41 ++++++++++++++++---
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
index 06e83ba128be3..3ced96af29564 100644
--- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
@@ -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" />});
     });
diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js
index 7a238391493a3..0c3797023cb04 100644
--- a/packages/react-server/src/ReactFizzServer.js
+++ b/packages/react-server/src/ReactFizzServer.js
@@ -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(request, 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
@@ -1430,6 +1430,31 @@ function abortTask(task: Task): void {
   }
 }
 
+function queueCompletedSegment(
+  request: Request,
+  boundary: SuspenseBoundary,
+  segment: Segment,
+): void {
+  if (
+    segment.chunks.length === 0 &&
+    segment.boundary === null &&
+    segment.children.length > 0
+  ) {
+    // This is an empty segment. There's nothing to write, so we can instead mark the
+    // children to be written.
+    for (let i = 0; i < segment.children.length; i++) {
+      const childSegment = segment.children[i];
+      childSegment.parentFlushed = true;
+      if (childSegment.status === COMPLETED) {
+        queueCompletedSegment(request, boundary, childSegment);
+      }
+    }
+  } else {
+    const completedSegments = boundary.completedSegments;
+    completedSegments.push(segment);
+  }
+}
+
 function finishedTask(
   request: Request,
   boundary: Root | SuspenseBoundary,
@@ -1463,7 +1488,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(request, boundary, segment);
         }
       }
       if (boundary.parentFlushed) {
@@ -1483,14 +1508,18 @@ function finishedTask(
         // to emit it.
         if (segment.status === COMPLETED) {
           const completedSegments = boundary.completedSegments;
-          completedSegments.push(segment);
-          if (completedSegments.length === 1) {
+          if (completedSegments.length === 0) {
+            queueCompletedSegment(request, boundary, segment);
             // 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
             // in case the parent has already been flushed.
-            if (boundary.parentFlushed) {
-              request.partialBoundaries.push(boundary);
+            if (completedSegments.length > 0) {
+              if (boundary.parentFlushed) {
+                request.partialBoundaries.push(boundary);
+              }
             }
+          } else {
+            queueCompletedSegment(request, boundary, segment);
           }
         }
       }