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

)"" This reverts commit cfd9d8c.
facebook · Apr 19, 2023 · f923179 · f923179
1 parent d08b6cf
commit f923179
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 65 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 start a new suspended commit after a previous one finishes', async () => {
+  it('can interrupt a suspended commit with a new transition', async () => {
     function App({children}) {
       return (
         <html>
@@ -3225,81 +3225,66 @@ body {
       );
     }
     const root = ReactDOMClient.createRoot(document);
-    root.render(<App />);
+    root.render(<App>(empty)</App>);
+
+    // Start a transition to "A"
     React.startTransition(() => {
       root.render(
         <App>
-          hello
-          <link rel="stylesheet" href="foo" precedence="default" />
+          A
+          <link rel="stylesheet" href="A" 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="foo" as="style" />
+          <link rel="preload" href="A" as="style" />
         </head>
-        <body />
+        <body>(empty)</body>
       </html>,
     );
 
+    // Interrupt the "A" transition with a new one, "B"
     React.startTransition(() => {
       root.render(
         <App>
-          hello2
-          {null}
-          <link rel="stylesheet" href="bar" precedence="default" />
+          B
+          <link rel="stylesheet" href="B" precedence="default" />
         </App>,
       );
     });
     await waitForAll([]);
-    expect(getMeaningfulChildren(document)).toEqual(
-      <html>
-        <head>
-          <link rel="preload" href="foo" as="style" />
-        </head>
-        <body />
-      </html>,
-    );
 
-    loadPreloads();
-    loadStylesheets();
-    assertLog(['load preload: foo', 'load stylesheet: foo']);
+    // Still on the initial UI because "B" hasn't loaded, but its preload
+    // is now in the head, too.
     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="A" as="style" />
+          <link rel="preload" href="B" as="style" />
         </head>
-        <body>hello</body>
+        <body>(empty)</body>
       </html>,
     );
 
-    // 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>,
-    );
+    // Finish loading
     loadPreloads();
     loadStylesheets();
-    assertLog(['load preload: bar', 'load stylesheet: bar']);
+    assertLog(['load preload: A', 'load preload: B', 'load stylesheet: B']);
+    // The "B" transition has finished.
     expect(getMeaningfulChildren(document)).toEqual(
       <html>
         <head>
-          <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" />
+          <link rel="stylesheet" href="B" data-precedence="default" />
+          <link rel="preload" href="A" as="style" />
+          <link rel="preload" href="B" as="style" />
         </head>
-        <body>hello2</body>
+        <body>B</body>
       </html>,
     );
   });

diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js
@@ -18,7 +18,6 @@ import {
   SyncLane,
   getHighestPriorityLane,
   getNextLanes,
-  includesOnlyNonUrgentLanes,
   includesSyncLane,
   markStarvedLanesAsExpired,
 } from './ReactFiberLane';
@@ -301,14 +300,16 @@ 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.
-    (isWorkLoopSuspendedOnData() && root === workInProgressRoot) ||
-    // We should only interrupt a pending commit if the new update
-    // is urgent.
-    (root.cancelPendingCommit !== null && includesOnlyNonUrgentLanes(nextLanes))
+    //
+    // Suspended render phase
+    (root === workInProgressRoot && isWorkLoopSuspendedOnData()) ||
+    // Suspended commit phase
+    root.cancelPendingCommit !== null
   ) {
     // 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,8 +725,11 @@ export function scheduleUpdateOnFiber(
   // Check if the work loop is currently suspended and waiting for data to
   // finish loading.
   if (
-    workInProgressSuspendedReason === SuspendedOnData &&
-    root === workInProgressRoot
+    // Suspended render phase
+    (root === workInProgressRoot &&
+      workInProgressSuspendedReason === SuspendedOnData) ||
+    // Suspended commit phase
+    root.cancelPendingCommit !== null
   ) {
     // 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 urgent update, it should interrupt the suspended commit.
+    // If there's an 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 non-urgent update does not interrupt a suspended commit', async () => {
+  test('a transition update interrupts a suspended commit', async () => {
     const root = ReactNoop.createRoot();
 
     // Mount an image. This transition will suspend because it's not inside a
@@ -159,26 +159,12 @@ describe('ReactSuspenseyCommitPhase', () => {
     // Nothing showing yet.
     expect(root).toMatchRenderedOutput(null);
 
-    // If there's another transition update, it should not interrupt the
-    // suspended commit.
+    // If there's an update, it should 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');
   });