From cfd9d8c73fef14fc3d903fc5bd691e65d40f87d9 Mon Sep 17 00:00:00 2001 From: Jan Kassens Date: Mon, 17 Apr 2023 16:24:40 -0400 Subject: [PATCH] Revert "Allow transitions to interrupt Suspensey commits (#26531)" This reverts commit 888874673f81c08d9c3cfd4a56e2e93fd728894c. --- .../src/__tests__/ReactDOMFloat-test.js | 69 +++++++++++-------- .../src/ReactFiberRootScheduler.js | 11 ++- .../src/ReactFiberWorkLoop.js | 7 +- .../ReactSuspenseyCommitPhase-test.js | 20 +++++- 4 files changed, 66 insertions(+), 41 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js index d089b25ffa..5bdca5d43f 100644 --- 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 ( @@ -3225,66 +3225,81 @@ body { ); } const root = ReactDOMClient.createRoot(document); - root.render((empty)); - - // Start a transition to "A" + root.render(); React.startTransition(() => { root.render( - A - + hello + , ); }); 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( - + - (empty) + , ); - // Interrupt the "A" transition with a new one, "B" React.startTransition(() => { root.render( - B - + hello2 + {null} + , ); }); await waitForAll([]); - - // Still on the initial UI because "B" hasn't loaded, but its preload - // is now in the head, too. expect(getMeaningfulChildren(document)).toEqual( - - + - (empty) + , ); - // Finish loading loadPreloads(); loadStylesheets(); - assertLog(['load preload: A', 'load preload: B', 'load stylesheet: B']); - // The "B" transition has finished. + assertLog(['load preload: foo', 'load stylesheet: foo']); expect(getMeaningfulChildren(document)).toEqual( - - - + + - B + hello + , + ); + + // The second update should process now + await waitForAll([]); + expect(getMeaningfulChildren(document)).toEqual( + + + + + + + hello + , + ); + loadPreloads(); + loadStylesheets(); + assertLog(['load preload: bar', 'load stylesheet: bar']); + expect(getMeaningfulChildren(document)).toEqual( + + + + + + + + hello2 , ); }); diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js index be14c46950..5068194aa2 100644 --- 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 index 71e6f4feab..55c30ae81a 100644 --- 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 index 2a558ba93a..bbd2ae8c2e 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js @@ -137,7 +137,7 @@ 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(); }); @@ -145,7 +145,7 @@ describe('ReactSuspenseyCommitPhase', () => { 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(); }); }); + // Still suspended. + expect(root).toMatchRenderedOutput(null); + + await act(() => { + // Resolving the image should result in an immediate, synchronous commit. + resolveSuspenseyThing('A'); + expect(root).toMatchRenderedOutput(); + }); + // 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'); });