Bugfix: Suspending in shell during discrete update (#25495)

acdlite · sebmarkbage · web-flow · commit 9ecf84ed7ffb · 2022-10-17T14:17:17.000-04:00
Fixes a bug that happens when you suspend in the shell (the part of the
tree that is not wrapped in a Suspense boundary) during a
discrete update.

There were two underyling issues. One was just a mistake:
RootDidNotComplete needs to be handled in both renderRootConcurrent and
renderRootSync, but it was only handled in renderRootConcurrent. I did
it this way because I thought this path was unreachable during a sync
update, but I neglected to consider that renderRootSync is sometimes
called for non-concurrent lanes, like when recovering from an error, or
patching up a mutation to an external store.

After I fixed that oversight, the other issue is that we intentionally
error if the shell suspends during a sync update. The idea was that you
should either wrap the tree in a Suspense boundary, or you should mark
the update as a transition to allow React to suspend.

However, this did not take into account selective hydration, which can
force a sync render before anything has even committed. There's no way
in that case to wrap the update in startTransition.

Our solution for now is to remove the error that happens when you
suspend in the shell during a sync update — even for discrete updates.

We will likely revisit this in the future. One appealing possibility is
to commit the whole root in an inert state, as if it were a hidden
Offscreen tree.

Co-authored-by: Sebastian Markbåge &lt;sebastian@calyptus.eu&gt;

Co-authored-by: Sebastian Markbåge &lt;sebastian@calyptus.eu&gt;
diff --git a/packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js
@@ -1695,4 +1695,81 @@ describe('ReactDOMServerSelectiveHydration', () => {
     expect(triggeredParent).toBe(false);
     expect(triggeredChild).toBe(false);
   });
+
+  it('can attempt sync hydration if suspended root is still concurrently rendering', async () => {
+    let suspend = false;
+    let resolve;
+    const promise = new Promise(resolvePromise => (resolve = resolvePromise));
+    function Child({text}) {
+      if (suspend) {
+        throw promise;
+      }
+      Scheduler.unstable_yieldValue(text);
+      return (
+        <span
+          onClick={e => {
+            e.preventDefault();
+            Scheduler.unstable_yieldValue('Clicked ' + text);
+          }}>
+          {text}
+        </span>
+      );
+    }
+
+    function App() {
+      Scheduler.unstable_yieldValue('App');
+      return (
+        <div>
+          <Child text="A" />
+        </div>
+      );
+    }
+
+    const finalHTML = ReactDOMServer.renderToString(<App />);
+
+    expect(Scheduler).toHaveYielded(['App', 'A']);
+
+    const container = document.createElement('div');
+    // We need this to be in the document since we'll dispatch events on it.
+    document.body.appendChild(container);
+
+    container.innerHTML = finalHTML;
+
+    const span = container.getElementsByTagName('span')[0];
+
+    // We suspend on the client.
+    suspend = true;
+
+    React.startTransition(() => {
+      ReactDOMClient.hydrateRoot(container, <App />);
+    });
+    expect(Scheduler).toFlushAndYieldThrough(['App']);
+
+    // This should attempt to synchronously hydrate the root, then pause
+    // because it still suspended
+    const result = dispatchClickEvent(span);
+    expect(Scheduler).toHaveYielded(['App']);
+    // The event should not have been cancelled because we didn't hydrate.
+    expect(result).toBe(true);
+
+    // Finish loading the data
+    await act(async () => {
+      suspend = false;
+      await resolve();
+    });
+
+    // The app should have successfully hydrated and rendered
+    if (
+      gate(
+        flags =>
+          flags.enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay,
+      )
+    ) {
+      expect(Scheduler).toHaveYielded(['App', 'A']);
+    } else {
+      expect(Scheduler).toHaveYielded(['App', 'A', 'Clicked A']);
+    }
+
+    document.body.removeChild(container);
+  });
 });
diff --git a/packages/react-reconciler/src/ReactFiberThrow.new.js b/packages/react-reconciler/src/ReactFiberThrow.new.js
@@ -69,13 +69,13 @@ import {
   includesSomeLane,
   mergeLanes,
   pickArbitraryLane,
-  includesSyncLane,
 } from './ReactFiberLane.new';
 import {
   getIsHydrating,
   markDidThrowWhileHydratingDEV,
   queueHydrationError,
 } from './ReactFiberHydrationContext.new';
+import {ConcurrentRoot} from './ReactRootTags';
 
 function createRootErrorUpdate(
   fiber: Fiber,
@@ -421,32 +421,26 @@ function throwException(
       // No boundary was found. Unless this is a sync update, this is OK.
       // We can suspend and wait for more data to arrive.
 
-      if (!includesSyncLane(rootRenderLanes)) {
-        // This is not a sync update. Suspend. Since we're not activating a
-        // Suspense boundary, this will unwind all the way to the root without
-        // performing a second pass to render a fallback. (This is arguably how
-        // refresh transitions should work, too, since we're not going to commit
-        // the fallbacks anyway.)
+      if (root.tag === ConcurrentRoot) {
+        // In a concurrent root, suspending without a Suspense boundary is
+        // allowed. It will suspend indefinitely without committing.
         //
-        // This case also applies to initial hydration.
+        // TODO: Should we have different behavior for discrete updates? What
+        // about flushSync? Maybe it should put the tree into an inert state,
+        // and potentially log a warning. Revisit this for a future release.
         attachPingListener(root, wakeable, rootRenderLanes);
         renderDidSuspendDelayIfPossible();
         return;
+      } else {
+        // In a legacy root, suspending without a boundary is always an error.
+        const uncaughtSuspenseError = new Error(
+          'A component suspended while responding to synchronous input. This ' +
+            'will cause the UI to be replaced with a loading indicator. To ' +
+            'fix, updates that suspend should be wrapped ' +
+            'with startTransition.',
+        );
+        value = uncaughtSuspenseError;
       }
-
-      // This is a sync/discrete update. We treat this case like an error
-      // because discrete renders are expected to produce a complete tree
-      // synchronously to maintain consistency with external state.
-      const uncaughtSuspenseError = new Error(
-        'A component suspended while responding to synchronous input. This ' +
-          'will cause the UI to be replaced with a loading indicator. To ' +
-          'fix, updates that suspend should be wrapped ' +
-          'with startTransition.',
-      );
-
-      // If we're outside a transition, fall through to the regular error path.
-      // The error will be caught by the nearest suspense boundary.
-      value = uncaughtSuspenseError;
     }
   } else {
     // This is a regular error, not a Suspense wakeable.
diff --git a/packages/react-reconciler/src/ReactFiberThrow.old.js b/packages/react-reconciler/src/ReactFiberThrow.old.js
@@ -69,13 +69,13 @@ import {
   includesSomeLane,
   mergeLanes,
   pickArbitraryLane,
-  includesSyncLane,
 } from './ReactFiberLane.old';
 import {
   getIsHydrating,
   markDidThrowWhileHydratingDEV,
   queueHydrationError,
 } from './ReactFiberHydrationContext.old';
+import {ConcurrentRoot} from './ReactRootTags';
 
 function createRootErrorUpdate(
   fiber: Fiber,
@@ -421,32 +421,26 @@ function throwException(
       // No boundary was found. Unless this is a sync update, this is OK.
       // We can suspend and wait for more data to arrive.
 
-      if (!includesSyncLane(rootRenderLanes)) {
-        // This is not a sync update. Suspend. Since we're not activating a
-        // Suspense boundary, this will unwind all the way to the root without
-        // performing a second pass to render a fallback. (This is arguably how
-        // refresh transitions should work, too, since we're not going to commit
-        // the fallbacks anyway.)
+      if (root.tag === ConcurrentRoot) {
+        // In a concurrent root, suspending without a Suspense boundary is
+        // allowed. It will suspend indefinitely without committing.
         //
-        // This case also applies to initial hydration.
+        // TODO: Should we have different behavior for discrete updates? What
+        // about flushSync? Maybe it should put the tree into an inert state,
+        // and potentially log a warning. Revisit this for a future release.
         attachPingListener(root, wakeable, rootRenderLanes);
         renderDidSuspendDelayIfPossible();
         return;
+      } else {
+        // In a legacy root, suspending without a boundary is always an error.
+        const uncaughtSuspenseError = new Error(
+          'A component suspended while responding to synchronous input. This ' +
+            'will cause the UI to be replaced with a loading indicator. To ' +
+            'fix, updates that suspend should be wrapped ' +
+            'with startTransition.',
+        );
+        value = uncaughtSuspenseError;
       }
-
-      // This is a sync/discrete update. We treat this case like an error
-      // because discrete renders are expected to produce a complete tree
-      // synchronously to maintain consistency with external state.
-      const uncaughtSuspenseError = new Error(
-        'A component suspended while responding to synchronous input. This ' +
-          'will cause the UI to be replaced with a loading indicator. To ' +
-          'fix, updates that suspend should be wrapped ' +
-          'with startTransition.',
-      );
-
-      // If we're outside a transition, fall through to the regular error path.
-      // The error will be caught by the nearest suspense boundary.
-      value = uncaughtSuspenseError;
     }
   } else {
     // This is a regular error, not a Suspense wakeable.
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -1063,10 +1063,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
       // The render unwound without completing the tree. This happens in special
       // cases where need to exit the current render without producing a
       // consistent tree or committing.
-      //
-      // This should only happen during a concurrent render, not a discrete or
-      // synchronous update. We should have already checked for this when we
-      // unwound the stack.
       markRootSuspended(root, lanes);
     } else {
       // The render completed.
@@ -1111,6 +1107,9 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
           ensureRootIsScheduled(root, now());
           throw fatalError;
         }
+
+        // FIXME: Need to check for RootDidNotComplete again. The factoring here
+        // isn't ideal.
       }
 
       // We now have a consistent tree. The next step is either to commit it,
@@ -1473,7 +1472,12 @@ function performSyncWorkOnRoot(root) {
   }
 
   if (exitStatus === RootDidNotComplete) {
-    throw new Error('Root did not complete. This is a bug in React.');
+    // The render unwound without completing the tree. This happens in special
+    // cases where need to exit the current render without producing a
+    // consistent tree or committing.
+    markRootSuspended(root, lanes);
+    ensureRootIsScheduled(root, now());
+    return null;
   }
 
   // We now have a consistent tree. Because this is a sync render, we
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -1063,10 +1063,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
       // The render unwound without completing the tree. This happens in special
       // cases where need to exit the current render without producing a
       // consistent tree or committing.
-      //
-      // This should only happen during a concurrent render, not a discrete or
-      // synchronous update. We should have already checked for this when we
-      // unwound the stack.
       markRootSuspended(root, lanes);
     } else {
       // The render completed.
@@ -1111,6 +1107,9 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
           ensureRootIsScheduled(root, now());
           throw fatalError;
         }
+
+        // FIXME: Need to check for RootDidNotComplete again. The factoring here
+        // isn't ideal.
       }
 
       // We now have a consistent tree. The next step is either to commit it,
@@ -1473,7 +1472,12 @@ function performSyncWorkOnRoot(root) {
   }
 
   if (exitStatus === RootDidNotComplete) {
-    throw new Error('Root did not complete. This is a bug in React.');
+    // The render unwound without completing the tree. This happens in special
+    // cases where need to exit the current render without producing a
+    // consistent tree or committing.
+    markRootSuspended(root, lanes);
+    ensureRootIsScheduled(root, now());
+    return null;
   }
 
   // We now have a consistent tree. Because this is a sync render, we
diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
@@ -1012,15 +1012,21 @@ describe('ReactSuspenseWithNoopRenderer', () => {
   });
 
   // @gate enableCache
-  it('errors when an update suspends without a placeholder during a sync update', () => {
-    // This is an error because sync/discrete updates are expected to produce
-    // a complete tree immediately to maintain consistency with external state
-    // — we can't delay the commit.
+  it('in concurrent mode, does not error when an update suspends without a Suspense boundary during a sync update', () => {
+    // NOTE: We may change this to be a warning in the future.
     expect(() => {
       ReactNoop.flushSync(() => {
         ReactNoop.render(<AsyncText text="Async" />);
       });
-    }).toThrow('A component suspended while responding to synchronous input.');
+    }).not.toThrow();
+  });
+
+  // @gate enableCache
+  it('in legacy mode, errors when an update suspends without a Suspense boundary during a sync update', () => {
+    const root = ReactNoop.createLegacyRoot();
+    expect(() => root.render(<AsyncText text="Async" />)).toThrow(
+      'A component suspended while responding to synchronous input.',
+    );
   });
 
   // @gate enableCache
