diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 2231dc5ae5b6..fdd173199e60 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -98,6 +98,8 @@ import { } from './ReactUpdateQueue'; import { NoWork, + Idle, + ContinuousHydration, Never, Sync, computeAsyncExpiration, @@ -1590,6 +1592,42 @@ function shouldRemainOnFallback( ); } +function getRemainingWorkInPrimaryTree( + workInProgress, + currentChildExpirationTime, + renderExpirationTime, +) { + if (currentChildExpirationTime < renderExpirationTime) { + // The highest priority remaining work is not part of this render. So the + // remaining work has not changed. + return currentChildExpirationTime; + } + if ((workInProgress.mode & BlockingMode) !== NoMode) { + // The highest priority remaining work is part of this render. Since we only + // keep track of the highest level, we don't know if there's a lower + // priority level scheduled. As a compromise, we'll render at a really low + // priority that includes all the updates. + // TODO: If expirationTime were a bitmask where each bit represents a + // separate task thread, this would be: currentChildBits & ~renderBits + // TODO: We used to track the lowest pending level for the whole root, but + // we removed it to simplify the implementation. It might be worth adding + // it back for this use case, to avoid redundant passes. + if (renderExpirationTime > ContinuousHydration) { + // First we try ContinuousHydration, so that we don't get grouped + // with Idle. + return ContinuousHydration; + } else if (renderExpirationTime > Idle) { + // Then we'll try Idle. + return Idle; + } else { + // Already at lowest possible update level. + return NoWork; + } + } + // In legacy mode, there's no work left. + return NoWork; +} + function updateSuspenseComponent( current, workInProgress, @@ -1831,8 +1869,15 @@ function updateSuspenseComponent( fallbackChildFragment.return = workInProgress; primaryChildFragment.sibling = fallbackChildFragment; fallbackChildFragment.effectTag |= Placement; - primaryChildFragment.childExpirationTime = NoWork; - + primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree( + workInProgress, + // This argument represents the remaining work in the current + // primary tree. Since the current tree did not already time out + // the direct parent of the primary children is the Suspense + // fiber, not a fragment. + current.childExpirationTime, + renderExpirationTime, + ); workInProgress.memoizedState = SUSPENDED_MARKER; workInProgress.child = primaryChildFragment; @@ -1895,6 +1940,11 @@ function updateSuspenseComponent( ); fallbackChildFragment.return = workInProgress; primaryChildFragment.sibling = fallbackChildFragment; + primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree( + workInProgress, + currentPrimaryChildFragment.childExpirationTime, + renderExpirationTime, + ); primaryChildFragment.childExpirationTime = NoWork; // Skip the primary children, and continue working on the // fallback children. @@ -1989,7 +2039,15 @@ function updateSuspenseComponent( fallbackChildFragment.return = workInProgress; primaryChildFragment.sibling = fallbackChildFragment; fallbackChildFragment.effectTag |= Placement; - primaryChildFragment.childExpirationTime = NoWork; + primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree( + workInProgress, + // This argument represents the remaining work in the current + // primary tree. Since the current tree did not already time out + // the direct parent of the primary children is the Suspense + // fiber, not a fragment. + current.childExpirationTime, + renderExpirationTime, + ); // Skip the primary children, and continue working on the // fallback children. workInProgress.memoizedState = SUSPENDED_MARKER; diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index e24008eb54d2..5cecd89b00a8 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -734,12 +734,22 @@ describe('ReactHooksWithNoopRenderer', () => { root.render(); setLabel('B'); }); - expect(Scheduler).toHaveYielded(['Suspend!']); + expect(Scheduler).toHaveYielded([ + 'Suspend!', + // TODO: This second attempt is because React isn't sure if there's + // another update at a lower priority. Ideally we wouldn't need it. + 'Suspend!', + ]); expect(root).toMatchRenderedOutput(); // Rendering again should suspend again. root.render(); - expect(Scheduler).toFlushAndYield(['Suspend!']); + expect(Scheduler).toFlushAndYield([ + 'Suspend!', + // TODO: This second attempt is because React isn't sure if there's + // another update at a lower priority. Ideally we wouldn't need it. + 'Suspend!', + ]); // Flip the signal back to "cancel" the update. However, the update to // label should still proceed. It shouldn't have been dropped. diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 0a1d209d41f5..9029a911cc49 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -956,7 +956,14 @@ describe('ReactSuspense', () => { // Update that suspends instance.setState({step: 2}); - expect(Scheduler).toFlushAndYield(['Suspend! [Step: 2]', 'Loading...']); + expect(Scheduler).toFlushAndYield([ + 'Suspend! [Step: 2]', + 'Loading...', + // TODO: This second attempt is because React isn't sure if there's + // another update at a lower priority. Ideally we wouldn't need it. + 'Suspend! [Step: 2]', + 'Loading...', + ]); jest.advanceTimersByTime(500); expect(root).toMatchRenderedOutput('Loading...'); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index fa7c72a3aa22..594bffe77e6e 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -2842,6 +2842,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { foo.setState({suspend: false}); }); + expect(Scheduler).toHaveYielded(['Foo']); expect(root).toMatchRenderedOutput(); }); @@ -2957,4 +2958,142 @@ describe('ReactSuspenseWithNoopRenderer', () => { , ); }); + + it( + 'multiple updates originating inside a Suspense boundary at different ' + + 'priority levels are not dropped', + async () => { + const {useState} = React; + const root = ReactNoop.createRoot(); + + function Parent() { + return ( + <> + }> + + + + ); + } + + let setText; + let setShouldHide; + function Child() { + const [text, _setText] = useState('A'); + const [shouldHide, _setShouldHide] = useState(false); + setText = _setText; + setShouldHide = _setShouldHide; + return shouldHide ? : ; + } + + await ReactNoop.act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Suspend! [A]', 'Loading...']); + expect(root).toMatchRenderedOutput(); + + await resolveText('A'); + expect(Scheduler).toHaveYielded(['Promise resolved [A]']); + expect(Scheduler).toFlushAndYield(['A']); + expect(root).toMatchRenderedOutput(); + + await ReactNoop.act(async () => { + // Schedule two updates that originate inside the Suspense boundary. + // The first one causes the boundary to suspend. The second one is at + // lower priority and unsuspends it by hiding the async component. + ReactNoop.discreteUpdates(() => { + setText('B'); + }); + setShouldHide(true); + }); + expect(Scheduler).toHaveYielded([ + // First we attempt the high pri update. It suspends. + 'Suspend! [B]', + 'Loading...', + // Then we attempt the low pri update, which finishes successfully. + '(empty)', + ]); + expect(root).toMatchRenderedOutput(); + }, + ); + + it( + 'multiple updates originating inside a Suspense boundary at different ' + + 'priority levels are not dropped, including Idle updates', + async () => { + const {useState} = React; + const root = ReactNoop.createRoot(); + + function Parent() { + return ( + <> + }> + + + + ); + } + + let setText; + let setShouldHide; + function Child() { + const [text, _setText] = useState('A'); + const [shouldHide, _setShouldHide] = useState(false); + setText = _setText; + setShouldHide = _setShouldHide; + return shouldHide ? : ; + } + + await ReactNoop.act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Suspend! [A]', 'Loading...']); + expect(root).toMatchRenderedOutput(); + + await resolveText('A'); + expect(Scheduler).toHaveYielded(['Promise resolved [A]']); + expect(Scheduler).toFlushAndYield(['A']); + expect(root).toMatchRenderedOutput(); + + await ReactNoop.act(async () => { + // Schedule two updates that originate inside the Suspense boundary. + // The first one causes the boundary to suspend. The second one is at + // lower priority and unsuspends it by hiding the async component. + setText('B'); + Scheduler.unstable_runWithPriority( + Scheduler.unstable_IdlePriority, + () => { + setShouldHide(true); + }, + ); + }); + expect(Scheduler).toHaveYielded([ + // First we attempt the high pri update. It suspends. + 'Suspend! [B]', + 'Loading...', + + // Then retry at a lower priority level. + // NOTE: The current implementation doesn't know which level to attempt, + // so it picks ContinuousHydration, which is one level higher than Idle. + // Since it doesn't include the Idle update, it will suspend again and + // reschedule to try at Idle. If we refactor expiration time to be a + // bitmask, we shouldn't need this heuristic. + 'Suspend! [B]', + 'Loading...', + ]); + + // Commit the placeholder to unblock the Idle update. + await advanceTimers(250); + expect(root).toMatchRenderedOutput( + <> +