Repository: facebook/react
Pull request: [Fizz] Render preamble eagerly

We unnecessarily render the preamble in a task. This updates the implementation to perform this render inline.

Testing this is tricky because one of the only ways you could assert this was even happening is based on how things error if you abort while rendering the root.

While adding a test for this I discovered that not all abortable tasks report errors when aborted during a normal render. I've asserted the current behavior and will address the other issue at another time and updated the assertion later as necessary

Changed files:
filename: packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
patch: @@ -9544,6 +9544,102 @@ describe('ReactDOMFizzServer', () => {
     );
   });
 
+  it('will attempt to render the preamble inline to allow rendering before a later abort in the same task', async () => {
+    const promise = new Promise(() => {});
+    function Pending() {
+      React.use(promise);
+    }
+
+    const controller = new AbortController();
+    function Abort() {
+      controller.abort();
+      return <Comp />;
+    }
+
+    function Comp() {
+      return null;
+    }
+
+    function App() {
+      return (
+        <html>
+          <head>
+            <meta content="here" />
+          </head>
+          <body>
+            <main>hello</main>
+            <Suspense>
+              <Pending />
+            </Suspense>
+            <Abort />
+          </body>
+        </html>
+      );
+    }
+
+    const signal = controller.signal;
+
+    let thrownError = null;
+    const errors = [];
+    try {
+      await act(() => {
+        const {pipe, abort} = renderToPipeableStream(<App />, {
+          onError(e, ei) {
+            errors.push({
+              error: e,
+              componentStack: normalizeCodeLocInfo(ei.componentStack),
+            });
+          },
+        });
+        signal.addEventListener('abort', () => abort('boom'));
+        pipe(writable);
+      });
+    } catch (e) {
+      thrownError = e;
+    }
+
+    expect(thrownError).toBe('boom');
+    // TODO there should actually be three errors. One for the pending Suspense, one for the fallback task, and one for the task
+    // that does the abort itself. At the moment abort will flush queues and if there is no pending tasks will close the request before
+    // the task which initiated the abort can even be processed. This is a bug but not one that I am fixing with the current change
+    // so I am asserting the current behavior
+    expect(errors).toEqual([
+      {
+        error: 'boom',
+        componentStack: componentStack([
+          'Pending',
+          'Suspense',
+          'body',
+          'html',
+          'App',
+        ]),
+      },
+      {
+        error: 'boom',
+        componentStack: componentStack([
+          'Suspense Fallback',
+          'body',
+          'html',
+          'App',
+        ]),
+        // }, {
+        //   error: 'boom',
+        //   componentStack: componentStack(['Abort', 'body', 'html', 'App'])
+      },
+    ]);
+
+    // We expect the render to throw before streaming anything so the default
+    // document is still loaded
+    expect(getVisibleChildren(document)).toEqual(
+      <html>
+        <head />
+        <body>
+          <div id="container" />
+        </body>
+      </html>,
+    );
+  });
+
   it('Will wait to flush Document chunks until all boundaries which might contain a preamble are errored or resolved', async () => {
     let rejectFirst;
     const firstPromise = new Promise((_, reject) => {

filename: packages/react-server/src/ReactFizzServer.js
patch: @@ -2206,7 +2206,7 @@ function renderSuspenseList(
 
 function renderPreamble(
   request: Request,
-  task: Task,
+  task: RenderTask,
   blockedSegment: Segment,
   node: ReactNodeList,
 ): void {
@@ -2219,28 +2219,21 @@ function renderPreamble(
     false,
   );
   blockedSegment.preambleChildren.push(preambleSegment);
-  // @TODO we can just attempt to render in the current task rather than spawning a new one
-  const preambleTask = createRenderTask(
-    request,
-    null,
-    node,
-    -1,
-    task.blockedBoundary,
-    preambleSegment,
-    task.blockedPreamble,
-    task.hoistableState,
-    request.abortableTasks,
-    task.keyPath,
-    task.formatContext,
-    task.context,
-    task.treeContext,
-    task.row,
-    task.componentStack,
-    !disableLegacyContext ? task.legacyContext : emptyContextObject,
-    __DEV__ ? task.debugTask : null,
-  );
-  pushComponentStack(preambleTask);
-  request.pingedTasks.push(preambleTask);
+  task.blockedSegment = preambleSegment;
+  try {
+    preambleSegment.status = RENDERING;
+    renderNode(request, task, node, -1);
+    pushSegmentFinale(
+      preambleSegment.chunks,
+      request.renderState,
+      preambleSegment.lastPushedText,
+      preambleSegment.textEmbedded,
+    );
+    preambleSegment.status = COMPLETED;
+    finishedSegment(request, task.blockedBoundary, preambleSegment);
+  } finally {
+    task.blockedSegment = blockedSegment;
+  }
 }
 
 function renderHostElement(
@@ -2292,7 +2285,8 @@ function renderHostElement(
       props,
     ));
     if (isPreambleContext(newContext)) {
-      renderPreamble(request, task, segment, children);
+      // $FlowFixMe: Refined
+      renderPreamble(request, (task: RenderTask), segment, children);
     } else {
       // We use the non-destructive form because if something suspends, we still
       // need to pop back up and finish this subtree of HTML.