Revert "Allow transitions to interrupt Suspensey commits (#26531)"

This reverts commit 8888746.
facebook · Apr 17, 2023 · cfd9d8c · cfd9d8c
1 parent 314f7d3
commit cfd9d8c
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 40 deletions.
diff --git a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js
@@ -3216,7 +3216,7 @@ body {
     );
   });
 
-  it('can interrupt a suspended commit with a new transition', async () => {
+  it('can start a new suspended commit after a previous one finishes', async () => {
     function App({children}) {
       return (
         <html>
@@ -3225,66 +3225,81 @@ body {
       );
     }
     const root = ReactDOMClient.createRoot(document);
-    root.render(<App>(empty)</App>);
-
-    // Start a transition to "A"
+    root.render(<App />);
     React.startTransition(() => {
       root.render(
         <App>
-          A
-          <link rel="stylesheet" href="A" precedence="default" />
+          hello
+          <link rel="stylesheet" href="foo" precedence="default" />
         </App>,
       );
     });
     await waitForAll([]);
-
-    // "A" hasn't loaded yet, so we remain on the initial UI. Its preload
-    // has been inserted into the head, though.
     expect(getMeaningfulChildren(document)).toEqual(
       <html>
         <head>
-          <link rel="preload" href="A" as="style" />
+          <link rel="preload" href="foo" as="style" />
         </head>
-        <body>(empty)</body>
+        <body />
       </html>,
     );
 
-    // Interrupt the "A" transition with a new one, "B"
     React.startTransition(() => {
       root.render(
         <App>
-          B
-          <link rel="stylesheet" href="B" precedence="default" />
+          hello2
+          {null}
+          <link rel="stylesheet" href="bar" precedence="default" />
         </App>,
       );
     });
     await waitForAll([]);
+    expect(getMeaningfulChildren(document)).toEqual(
+      <html>
+        <head>
+          <link rel="preload" href="foo" as="style" />
+        </head>
+        <body />
+      </html>,
+    );
 
-    // Still on the initial UI because "B" hasn't loaded, but its preload
-    // is now in the head, too.
+    loadPreloads();
+    loadStylesheets();
+    assertLog(['load preload: foo', 'load stylesheet: foo']);
     expect(getMeaningfulChildren(document)).toEqual(
       <html>
         <head>
-          <link rel="preload" href="A" as="style" />
-          <link rel="preload" href="B" as="style" />
+          <link rel="stylesheet" href="foo" data-precedence="default" />
+          <link rel="preload" href="foo" as="style" />
         </head>
-        <body>(empty)</body>
+        <body>hello</body>
       </html>,
     );
 
-    // Finish loading
+    // The second update should process now
+    await waitForAll([]);
+    expect(getMeaningfulChildren(document)).toEqual(
+      <html>
+        <head>
+          <link rel="stylesheet" href="foo" data-precedence="default" />
+          <link rel="preload" href="foo" as="style" />
+          <link rel="preload" href="bar" as="style" />
+        </head>
+        <body>hello</body>
+      </html>,
+    );
     loadPreloads();
     loadStylesheets();
-    assertLog(['load preload: A', 'load preload: B', 'load stylesheet: B']);
-    // The "B" transition has finished.
+    assertLog(['load preload: bar', 'load stylesheet: bar']);
     expect(getMeaningfulChildren(document)).toEqual(
       <html>
         <head>
-          <link rel="stylesheet" href="B" data-precedence="default" />
-          <link rel="preload" href="A" as="style" />
-          <link rel="preload" href="B" as="style" />
+          <link rel="stylesheet" href="foo" data-precedence="default" />
+          <link rel="stylesheet" href="bar" data-precedence="default" />
+          <link rel="preload" href="foo" as="style" />
+          <link rel="preload" href="bar" as="style" />
         </head>
-        <body>B</body>
+        <body>hello2</body>
       </html>,
     );
   });

diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js
@@ -18,6 +18,7 @@ import {
   SyncLane,
   getHighestPriorityLane,
   getNextLanes,
+  includesOnlyNonUrgentLanes,
   includesSyncLane,
   markStarvedLanesAsExpired,
 } from './ReactFiberLane';
@@ -291,16 +292,14 @@ function scheduleTaskForRootDuringMicrotask(
 
   const existingCallbackNode = root.callbackNode;
   if (
-    // Check if there's nothing to work on
     nextLanes === NoLanes ||
     // If this root is currently suspended and waiting for data to resolve, don't
     // schedule a task to render it. We'll either wait for a ping, or wait to
     // receive an update.
-    //
-    // Suspended render phase
-    (root === workInProgressRoot && isWorkLoopSuspendedOnData()) ||
-    // Suspended commit phase
-    root.cancelPendingCommit !== null
+    (isWorkLoopSuspendedOnData() && root === workInProgressRoot) ||
+    // We should only interrupt a pending commit if the new update
+    // is urgent.
+    (root.cancelPendingCommit !== null && includesOnlyNonUrgentLanes(nextLanes))
   ) {
     // Fast path: There's nothing to work on.
     if (existingCallbackNode !== null) {

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js
@@ -725,11 +725,8 @@ export function scheduleUpdateOnFiber(
   // Check if the work loop is currently suspended and waiting for data to
   // finish loading.
   if (
-    // Suspended render phase
-    (root === workInProgressRoot &&
-      workInProgressSuspendedReason === SuspendedOnData) ||
-    // Suspended commit phase
-    root.cancelPendingCommit !== null
+    workInProgressSuspendedReason === SuspendedOnData &&
+    root === workInProgressRoot
   ) {
     // The incoming update might unblock the current render. Interrupt the
     // current attempt and restart from the top.

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js
@@ -137,15 +137,15 @@ describe('ReactSuspenseyCommitPhase', () => {
     // Nothing showing yet.
     expect(root).toMatchRenderedOutput(null);
 
-    // If there's an update, it should interrupt the suspended commit.
+    // If there's an urgent update, it should interrupt the suspended commit.
     await act(() => {
       root.render(<Text text="Something else" />);
     });
     assertLog(['Something else']);
     expect(root).toMatchRenderedOutput('Something else');
   });
 
-  test('a transition update interrupts a suspended commit', async () => {
+  test('a non-urgent update does not interrupt a suspended commit', async () => {
     const root = ReactNoop.createRoot();
 
     // Mount an image. This transition will suspend because it's not inside a
@@ -159,12 +159,26 @@ describe('ReactSuspenseyCommitPhase', () => {
     // Nothing showing yet.
     expect(root).toMatchRenderedOutput(null);
 
-    // If there's an update, it should interrupt the suspended commit.
+    // If there's another transition update, it should not interrupt the
+    // suspended commit.
     await act(() => {
       startTransition(() => {
         root.render(<Text text="Something else" />);
       });
     });
+    // Still suspended.
+    expect(root).toMatchRenderedOutput(null);
+
+    await act(() => {
+      // Resolving the image should result in an immediate, synchronous commit.
+      resolveSuspenseyThing('A');
+      expect(root).toMatchRenderedOutput(<suspensey-thing src="A" />);
+    });
+    // Then the second transition is unblocked.
+    // TODO: Right now the only way to unsuspend a commit early is to proceed
+    // with the commit even if everything isn't ready. Maybe there should also
+    // be a way to abort a commit so that it can be interrupted by
+    // another transition.
     assertLog(['Something else']);
     expect(root).toMatchRenderedOutput('Something else');
   });