Repository: facebook/react
Pull request: [Fizz] Support deeply nested Suspense inside fallback

When deeply nested Suspense boundaries inside a fallback of another boundary resolve it is possible to encounter situations where you either attempt to flush an aborted Segment or you have a boundary without any root segment. We intended for both of these conditions to be impossible to arrive at legitimately however it turns out in this situation you can. The fix is two-fold

1. allow flushing aborted segments by simply skipping them. This does remove some protection against future misconfiguraiton of React because it is no longer an invariant that you hsould never attempt to flush an aborted segment but there are legitimate cases where this can come up and simply omitting the segment is fine b/c we know that the user will never observe this. A semantically better solution would be to avoid flushing boudaries inside an unneeded fallback but to do this we would need to track all boundaries inside a fallback or create back pointers which add to memory overhead and possibly make GC harder to do efficiently. By flushing extra we're maintaining status quo and only suffer in performance not with broken semantics.

2. when queuing completed segments allow for queueing aborted segments and if we are eliding the enqueued segment allow for child segments that are errored to be enqueued too. This will mean that we can maintain the invariant that a boundary must have a root segment the first time we flush it, it just might be aborted (see point 1 above).

This change has two seemingly similar test cases to exercise this fix. The reason we need both is that when you have empty segments you hit different code paths within Fizz and so each one (without this fix) triggers a different error pathway.

This change also includes a fix to our tests where we were not appropriately setting CSPnonce back to null at the start of each test so in some contexts scripts would not run for some tests

Changed files:
filename: packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
patch: @@ -88,6 +88,7 @@ describe('ReactDOMFizzServer', () => {
       setTimeout(cb);
     container = document.getElementById('container');
 
+    CSPnonce = null;
     Scheduler = require('scheduler');
     React = require('react');
     ReactDOM = require('react-dom');
@@ -10447,4 +10448,110 @@ describe('ReactDOMFizzServer', () => {
       </html>,
     );
   });
+
+  it('should not error when discarding deeply nested Suspense boundaries in a parent fallback partially complete before the parent boundary resolves', async () => {
+    let resolve1;
+    const promise1 = new Promise(r => (resolve1 = r));
+    let resolve2;
+    const promise2 = new Promise(r => (resolve2 = r));
+    const promise3 = new Promise(r => {});
+
+    function Use({children, promise}) {
+      React.use(promise);
+      return children;
+    }
+    function App() {
+      return (
+        <div>
+          <Suspense
+            fallback={
+              <div>
+                <Suspense fallback="Loading...">
+                  <div>
+                    <Use promise={promise1}>
+                      <div>
+                        <Suspense fallback="Loading more...">
+                          <div>
+                            <Use promise={promise3}>
+                              <div>deep fallback</div>
+                            </Use>
+                          </div>
+                        </Suspense>
+                      </div>
+                    </Use>
+                  </div>
+                </Suspense>
+              </div>
+            }>
+            <Use promise={promise2}>Success!</Use>
+          </Suspense>
+        </div>
+      );
+    }
+
+    await act(() => {
+      const {pipe} = renderToPipeableStream(<App />);
+      pipe(writable);
+    });
+
+    expect(getVisibleChildren(container)).toEqual(
+      <div>
+        <div>Loading...</div>
+      </div>,
+    );
+
+    await act(() => {
+      resolve1('resolved');
+      resolve2('resolved');
+    });
+
+    expect(getVisibleChildren(container)).toEqual(<div>Success!</div>);
+  });
+
+  it('should not error when discarding deeply nested Suspense boundaries in a parent fallback partially complete before the parent boundary resolves with empty segments', async () => {
+    let resolve1;
+    const promise1 = new Promise(r => (resolve1 = r));
+    let resolve2;
+    const promise2 = new Promise(r => (resolve2 = r));
+    const promise3 = new Promise(r => {});
+
+    function Use({children, promise}) {
+      React.use(promise);
+      return children;
+    }
+    function App() {
+      return (
+        <div>
+          <Suspense
+            fallback={
+              <Suspense fallback="Loading...">
+                <Use promise={promise1}>
+                  <Suspense fallback="Loading more...">
+                    <Use promise={promise3}>
+                      <div>deep fallback</div>
+                    </Use>
+                  </Suspense>
+                </Use>
+              </Suspense>
+            }>
+            <Use promise={promise2}>Success!</Use>
+          </Suspense>
+        </div>
+      );
+    }
+
+    await act(() => {
+      const {pipe} = renderToPipeableStream(<App />);
+      pipe(writable);
+    });
+
+    expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);
+
+    await act(() => {
+      resolve1('resolved');
+      resolve2('resolved');
+    });
+
+    expect(getVisibleChildren(container)).toEqual(<div>Success!</div>);
+  });
 });

filename: packages/react-dom/src/__tests__/ReactDOMFloat-test.js
patch: @@ -25,7 +25,7 @@ let SuspenseList;
 let textCache;
 let loadCache;
 let writable;
-const CSPnonce = null;
+let CSPnonce = null;
 let container;
 let buffer = '';
 let hasErrored = false;
@@ -69,6 +69,7 @@ describe('ReactDOMFloat', () => {
       setTimeout(cb);
     container = document.getElementById('container');
 
+    CSPnonce = null;
     React = require('react');
     ReactDOM = require('react-dom');
     ReactDOMClient = require('react-dom/client');

filename: packages/react-server/src/ReactFizzServer.js
patch: @@ -4918,7 +4918,11 @@ function queueCompletedSegment(
     const childSegment = segment.children[0];
     childSegment.id = segment.id;
     childSegment.parentFlushed = true;
-    if (childSegment.status === COMPLETED) {
+    if (
+      childSegment.status === COMPLETED ||
+      childSegment.status === ABORTED ||
+      childSegment.status === ERRORED
+    ) {
       queueCompletedSegment(boundary, childSegment);
     }
   } else {
@@ -4989,7 +4993,7 @@ function finishedTask(
         // Our parent segment already flushed, so we need to schedule this segment to be emitted.
         // 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) {
+        if (segment.status === COMPLETED || segment.status === ABORTED) {
           queueCompletedSegment(boundary, segment);
         }
       }
@@ -5058,7 +5062,7 @@ function finishedTask(
         // Our parent already flushed, so we need to schedule this segment to be emitted.
         // 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) {
+        if (segment.status === COMPLETED || segment.status === ABORTED) {
           queueCompletedSegment(boundary, segment);
           const completedSegments = boundary.completedSegments;
           if (completedSegments.length === 1) {
@@ -5575,6 +5579,9 @@ function flushSubtree(
       }
       return r;
     }
+    case ABORTED: {
+      return true;
+    }
     default: {
       throw new Error(
         'Aborted, errored or already flushed boundaries should not be flushed again. This is a bug in React.',