From 9ae639ed49f91f43a00024ab783a8ab37ef09d80 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 3 Nov 2022 16:15:26 -0400 Subject: [PATCH] use: Don't suspend if there are pending updates Before suspending, check if there are other pending updates that might possibly unblock the suspended component. If so, interrupt the current render and switch to working on that. This logic was already implemented for the old "throw a Promise" Suspense but has to be replicated for `use` because it suspends the work loop much earlier. I'm getting a little anxious about the divergence between the two Suspense patterns. I'm going to look into enabling the new behavior for the old pattern so that we can unify the implementations. --- .../src/ReactFiberWorkLoop.new.js | 14 ++- .../src/ReactFiberWorkLoop.old.js | 14 ++- .../src/__tests__/ReactThenable-test.js | 90 +++++++++++++++++++ 3 files changed, 116 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 59adc4c1b5ad0..9bc53e4f30e78 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -1849,7 +1849,7 @@ function handleThrow(root, thrownValue): void { function shouldAttemptToSuspendUntilDataResolves() { // TODO: We should be able to move the - // renderDidSuspend/renderDidSuspendWithDelay logic into this function, + // renderDidSuspend/renderDidSuspendDelayIfPossible logic into this function, // instead of repeating it in the complete phase. Or something to that effect. if (includesOnlyRetries(workInProgressRootRenderLanes)) { @@ -1857,6 +1857,18 @@ function shouldAttemptToSuspendUntilDataResolves() { return true; } + // Check if there are other pending updates that might possibly unblock this + // component from suspending. This mirrors the check in + // renderDidSuspendDelayIfPossible. We should attempt to unify them somehow. + if ( + includesNonIdleWork(workInProgressRootSkippedLanes) || + includesNonIdleWork(workInProgressRootInterleavedUpdatedLanes) + ) { + // Suspend normally. renderDidSuspendDelayIfPossible will handle + // interrupting the work loop. + return false; + } + // TODO: We should be able to remove the equivalent check in // finishConcurrentRender, and rely just on this one. if (includesOnlyTransitions(workInProgressRootRenderLanes)) { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 101004007e399..12293f0210e0f 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -1849,7 +1849,7 @@ function handleThrow(root, thrownValue): void { function shouldAttemptToSuspendUntilDataResolves() { // TODO: We should be able to move the - // renderDidSuspend/renderDidSuspendWithDelay logic into this function, + // renderDidSuspend/renderDidSuspendDelayIfPossible logic into this function, // instead of repeating it in the complete phase. Or something to that effect. if (includesOnlyRetries(workInProgressRootRenderLanes)) { @@ -1857,6 +1857,18 @@ function shouldAttemptToSuspendUntilDataResolves() { return true; } + // Check if there are other pending updates that might possibly unblock this + // component from suspending. This mirrors the check in + // renderDidSuspendDelayIfPossible. We should attempt to unify them somehow. + if ( + includesNonIdleWork(workInProgressRootSkippedLanes) || + includesNonIdleWork(workInProgressRootInterleavedUpdatedLanes) + ) { + // Suspend normally. renderDidSuspendDelayIfPossible will handle + // interrupting the work loop. + return false; + } + // TODO: We should be able to remove the equivalent check in // finishConcurrentRender, and rely just on this one. if (includesOnlyTransitions(workInProgressRootRenderLanes)) { diff --git a/packages/react-reconciler/src/__tests__/ReactThenable-test.js b/packages/react-reconciler/src/__tests__/ReactThenable-test.js index f0bca1420145c..06a11a2101225 100644 --- a/packages/react-reconciler/src/__tests__/ReactThenable-test.js +++ b/packages/react-reconciler/src/__tests__/ReactThenable-test.js @@ -5,6 +5,7 @@ let ReactNoop; let Scheduler; let act; let use; +let useState; let Suspense; let startTransition; let pendingTextRequests; @@ -18,6 +19,7 @@ describe('ReactThenable', () => { Scheduler = require('scheduler'); act = require('jest-react').act; use = React.use; + useState = React.useState; Suspense = React.Suspense; startTransition = React.startTransition; @@ -668,4 +670,92 @@ describe('ReactThenable', () => { expect(Scheduler).toHaveYielded(['Hi']); expect(root).toMatchRenderedOutput('Hi'); }); + + // @gate enableUseHook + test('does not suspend indefinitely if an interleaved update was skipped', async () => { + function Child({childShouldSuspend}) { + return ( + + ); + } + + let setChildShouldSuspend; + let setShowChild; + function Parent() { + const [showChild, _setShowChild] = useState(true); + setShowChild = _setShowChild; + + const [childShouldSuspend, _setChildShouldSuspend] = useState(false); + setChildShouldSuspend = _setChildShouldSuspend; + + Scheduler.unstable_yieldValue( + `childShouldSuspend: ${childShouldSuspend}, showChild: ${showChild}`, + ); + return showChild ? ( + + ) : ( + + ); + } + + const root = ReactNoop.createRoot(); + await act(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded([ + 'childShouldSuspend: false, showChild: true', + 'Child', + ]); + expect(root).toMatchRenderedOutput('Child'); + + await act(() => { + // Perform an update that causes the app to suspend + startTransition(() => { + setChildShouldSuspend(true); + }); + expect(Scheduler).toFlushAndYieldThrough([ + 'childShouldSuspend: true, showChild: true', + ]); + // While the update is in progress, schedule another update. + startTransition(() => { + setShowChild(false); + }); + }); + expect(Scheduler).toHaveYielded([ + // Because the interleaved update is not higher priority than what we were + // already working on, it won't interrupt. The first update will continue, + // and will suspend. + 'Async text requested [Will never resolve]', + + // Instead of waiting for the promise to resolve, React notices there's + // another pending update that it hasn't tried yet. It will switch to + // rendering that instead. + // + // This time, the update hides the component that previous was suspending, + // so it finishes successfully. + 'childShouldSuspend: false, showChild: false', + '(empty)', + + // Finally, React attempts to render the first update again. It also + // finishes successfully, because it was rebased on top of the update that + // hid the suspended component. + // NOTE: These this render happened to not be entangled with the previous + // one. If they had been, this update would have been included in the + // previous render, and there wouldn't be an extra one here. This could + // change if we change our entanglement heurstics. Semantically, it + // shouldn't matter, though in general we try to work on transitions in + // parallel whenever possible. So even though in this particular case, the + // extra render is unnecessary, it's a nice property that it wasn't + // entangled with the other transition. + 'childShouldSuspend: true, showChild: false', + '(empty)', + ]); + expect(root).toMatchRenderedOutput('(empty)'); + }); });