diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 2231dc5ae5b6..014782b2fd6d 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -179,6 +179,7 @@ import { scheduleUpdateOnFiber, renderDidSuspendDelayIfPossible, markUnprocessedUpdateTime, + getWorkInProgressRoot, } from './ReactFiberWorkLoop'; const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; @@ -1590,6 +1591,35 @@ 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 the lowest + // known level in the entire tree, since that will include everything. + // TODO: If expirationTime were a bitmask where each bit represents a + // separate task thread, this would be: currentChildBits & ~renderBits + const root = getWorkInProgressRoot(); + if (root !== null) { + const lastPendingTime = root.lastPendingTime; + if (lastPendingTime < renderExpirationTime) { + return lastPendingTime; + } + } + } + // In legacy mode, there's no work left. + return NoWork; +} + function updateSuspenseComponent( current, workInProgress, @@ -1831,8 +1861,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 +1932,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 +2031,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; @@ -3006,6 +3056,42 @@ function beginWork( renderExpirationTime, ); } else { + // The primary child fragment does not have pending work marked + // on it... + + // ...usually. There's an unfortunate edge case where the fragment + // fiber is not part of the return path of the children, so when + // an update happens, the fragment doesn't get marked during + // setState. This is something we should consider addressing when + // we refactor the Fiber data structure. (There's a test with more + // details; to find it, comment out the following block and see + // which one fails.) + // + // As a workaround, we need to recompute the `childExpirationTime` + // by bubbling it up from the next level of children. This is + // based on similar logic in `resetChildExpirationTime`. + let primaryChild = primaryChildFragment.child; + while (primaryChild !== null) { + const childUpdateExpirationTime = primaryChild.expirationTime; + const childChildExpirationTime = + primaryChild.childExpirationTime; + if ( + (childUpdateExpirationTime !== NoWork && + childUpdateExpirationTime >= renderExpirationTime) || + (childChildExpirationTime !== NoWork && + childChildExpirationTime >= renderExpirationTime) + ) { + // Found a child with an update with sufficient priority. + // Use the normal path to render the primary children again. + return updateSuspenseComponent( + current, + workInProgress, + renderExpirationTime, + ); + } + primaryChild = primaryChild.sibling; + } + pushSuspenseContext( workInProgress, setDefaultShallowSuspenseContext(suspenseStackCursor.current), diff --git a/packages/react-reconciler/src/ReactFiberRoot.js b/packages/react-reconciler/src/ReactFiberRoot.js index d035d31c764f..544dcd076414 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.js +++ b/packages/react-reconciler/src/ReactFiberRoot.js @@ -65,6 +65,8 @@ type BaseFiberRootProperties = {| callbackPriority: ReactPriorityLevel, // The earliest pending expiration time that exists in the tree firstPendingTime: ExpirationTime, + // The latest pending expiration time that exists in the tree + lastPendingTime: ExpirationTime, // The earliest suspended expiration time that exists in the tree firstSuspendedTime: ExpirationTime, // The latest suspended expiration time that exists in the tree @@ -122,6 +124,7 @@ function FiberRootNode(containerInfo, tag, hydrate) { this.callbackNode = null; this.callbackPriority = NoPriority; this.firstPendingTime = NoWork; + this.lastPendingTime = NoWork; this.firstSuspendedTime = NoWork; this.lastSuspendedTime = NoWork; this.nextKnownPendingLevel = NoWork; @@ -205,6 +208,10 @@ export function markRootUpdatedAtTime( if (expirationTime > firstPendingTime) { root.firstPendingTime = expirationTime; } + const lastPendingTime = root.lastPendingTime; + if (lastPendingTime === NoWork || expirationTime < lastPendingTime) { + root.lastPendingTime = expirationTime; + } // Update the range of suspended times. Treat everything lower priority or // equal to this update as unsuspended. @@ -232,6 +239,11 @@ export function markRootFinishedAtTime( ): void { // Update the range of pending times root.firstPendingTime = remainingExpirationTime; + if (remainingExpirationTime < root.lastPendingTime) { + // This usually means we've finished all the work, but it can also happen + // when something gets downprioritized during render, like a hidden tree. + root.lastPendingTime = remainingExpirationTime; + } // Update the range of suspended times. Treat everything higher priority or // equal to this update as unsuspended. diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js index 12a6d03a2313..7928ca491b08 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js @@ -293,7 +293,13 @@ describe('ReactSuspenseList', () => { await C.resolve(); - expect(Scheduler).toFlushAndYield(['C']); + expect(Scheduler).toFlushAndYield([ + // TODO: Ideally we wouldn't have to retry B. This is an implementation + // trade off. + 'Suspend! [B]', + + 'C', + ]); expect(ReactNoop).toMatchRenderedOutput( <> diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 7107a87c3efe..55793fb6c3b3 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -3,11 +3,12 @@ let ReactFeatureFlags; let Fragment; let ReactNoop; let Scheduler; -let ReactCache; let Suspense; +let textCache; -let TextResource; -let textResourceShouldFail; +let readText; +let resolveText; +let rejectText; describe('ReactSuspenseWithNoopRenderer', () => { if (!__EXPERIMENTAL__) { @@ -26,26 +27,74 @@ describe('ReactSuspenseWithNoopRenderer', () => { Fragment = React.Fragment; ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); - ReactCache = require('react-cache'); Suspense = React.Suspense; - TextResource = ReactCache.unstable_createResource( - ([text, ms = 0]) => { - return new Promise((resolve, reject) => - setTimeout(() => { - if (textResourceShouldFail) { - Scheduler.unstable_yieldValue(`Promise rejected [${text}]`); - reject(new Error('Failed to load: ' + text)); - } else { - Scheduler.unstable_yieldValue(`Promise resolved [${text}]`); - resolve(text); - } - }, ms), - ); - }, - ([text, ms]) => text, - ); - textResourceShouldFail = false; + textCache = new Map(); + + readText = text => { + const record = textCache.get(text); + if (record !== undefined) { + switch (record.status) { + case 'pending': + throw record.promise; + case 'rejected': + throw Error('Failed to load: ' + text); + case 'resolved': + return text; + } + } else { + let ping; + const promise = new Promise(resolve => (ping = resolve)); + const newRecord = { + status: 'pending', + ping: ping, + promise, + }; + textCache.set(text, newRecord); + throw promise; + } + }; + + resolveText = text => { + const record = textCache.get(text); + if (record !== undefined) { + if (record.status === 'pending') { + Scheduler.unstable_yieldValue(`Promise resolved [${text}]`); + record.ping(); + record.ping = null; + record.status = 'resolved'; + clearTimeout(record.promise._timer); + record.promise = null; + } + } else { + const newRecord = { + ping: null, + status: 'resolved', + promise: null, + }; + textCache.set(text, newRecord); + } + }; + + rejectText = text => { + const record = textCache.get(text); + if (record !== undefined) { + if (record.status === 'pending') { + Scheduler.unstable_yieldValue(`Promise rejected [${text}]`); + record.ping(); + record.status = 'rejected'; + clearTimeout(record.promise._timer); + record.promise = null; + } + } else { + const newRecord = { + ping: null, + status: 'rejected', + promise: null, + }; + textCache.set(text, newRecord); + } + }; }); // function div(...children) { @@ -83,12 +132,17 @@ describe('ReactSuspenseWithNoopRenderer', () => { function AsyncText(props) { const text = props.text; try { - TextResource.read([props.text, props.ms]); + readText(text); Scheduler.unstable_yieldValue(text); return ; } catch (promise) { if (typeof promise.then === 'function') { Scheduler.unstable_yieldValue(`Suspend! [${text}]`); + if (typeof props.ms === 'number' && promise._timer === undefined) { + promise._timer = setTimeout(() => { + resolveText(text); + }, props.ms); + } } else { Scheduler.unstable_yieldValue(`Error! [${text}]`); } @@ -279,7 +333,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([]); // Wait for data to resolve - await advanceTimers(100); + await resolveText('B'); // Renders successfully expect(Scheduler).toHaveYielded(['Promise resolved [B]']); expect(Scheduler).toFlushAndYield(['A', 'B', 'C', 'D']); @@ -329,10 +383,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toFlushAndYield(['Suspend! [Result]', 'Loading...']); expect(ReactNoop.getChildren()).toEqual([]); - textResourceShouldFail = true; - ReactNoop.expire(1000); - await advanceTimers(1000); - textResourceShouldFail = false; + await rejectText('Result'); expect(Scheduler).toHaveYielded(['Promise rejected [Result]']); @@ -382,10 +433,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toFlushAndYield(['Suspend! [Result]', 'Loading...']); expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); - textResourceShouldFail = true; - ReactNoop.expire(3000); - await advanceTimers(3000); - textResourceShouldFail = false; + await rejectText('Result'); expect(Scheduler).toHaveYielded(['Promise rejected [Result]']); expect(Scheduler).toFlushAndYield([ @@ -416,7 +464,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { // Initial mount ReactNoop.render(); expect(Scheduler).toFlushAndYield(['A', 'Suspend! [1]', 'Loading...']); - await advanceTimers(0); + await resolveText('1'); expect(Scheduler).toHaveYielded(['Promise resolved [1]']); expect(Scheduler).toFlushAndYield(['A', '1']); expect(ReactNoop.getChildren()).toEqual([span('A'), span('1')]); @@ -439,7 +487,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('B'), span('1')]); // Unblock the low-pri text and finish - await advanceTimers(0); + await resolveText('2'); expect(Scheduler).toHaveYielded(['Promise resolved [2]']); expect(ReactNoop.getChildren()).toEqual([span('B'), span('1')]); }); @@ -472,7 +520,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'B', 'Loading...']); expect(ReactNoop.getChildren()).toEqual([]); - await advanceTimers(0); + await resolveText('A'); expect(Scheduler).toHaveYielded(['Promise resolved [A]']); expect(Scheduler).toFlushAndYield(['A', 'B']); expect(ReactNoop.getChildren()).toEqual([span('A'), span('B')]); @@ -711,7 +759,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('Loading...'), span('Sync')]); // Once the promise resolves, we render the suspended view - await advanceTimers(0); + await resolveText('Async'); expect(Scheduler).toHaveYielded(['Promise resolved [Async]']); expect(Scheduler).toFlushAndYield(['Async']); expect(ReactNoop.getChildren()).toEqual([span('Async'), span('Sync')]); @@ -1212,7 +1260,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { const text = props.text; Scheduler.unstable_yieldValue('constructor'); try { - TextResource.read([props.text, props.ms]); + readText(text); this.state = {text}; } catch (promise) { if (typeof promise.then === 'function') { @@ -1245,7 +1293,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { ]); expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); - await advanceTimers(1000); + await resolveText('Hi'); expect(Scheduler).toHaveYielded(['Promise resolved [Hi]']); expect(Scheduler).toFlushExpired([ @@ -1287,6 +1335,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'Suspend! [Hi]', 'Loading...', // Re-render due to lifecycle update + 'Suspend! [Hi]', 'Loading...', ]); expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); @@ -1495,9 +1544,8 @@ describe('ReactSuspenseWithNoopRenderer', () => { } render() { const text = this.props.text; - const ms = this.props.ms; try { - TextResource.read([text, ms]); + readText(text); Scheduler.unstable_yieldValue(text); return ; } catch (promise) { @@ -1581,9 +1629,8 @@ describe('ReactSuspenseWithNoopRenderer', () => { }; }, [props.text]); const text = props.text; - const ms = props.ms; try { - TextResource.read([text, ms]); + readText(text); Scheduler.unstable_yieldValue(text); return ; } catch (promise) { @@ -1640,8 +1687,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { , ); - Scheduler.unstable_advanceTime(500); - await advanceTimers(500); + await resolveText('B'); expect(Scheduler).toHaveYielded(['Promise resolved [B]']); @@ -1690,8 +1736,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'Effect [Loading...]', ]); - Scheduler.unstable_advanceTime(500); - await advanceTimers(500); + await resolveText('B2'); expect(Scheduler).toHaveYielded(['Promise resolved [B2]']); @@ -2068,8 +2113,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { await ReactNoop.act(async () => show(true)); expect(Scheduler).toHaveYielded(['Suspend! [A]']); - Scheduler.unstable_advanceTime(100); - await advanceTimers(100); + await resolveText('A'); expect(Scheduler).toHaveYielded(['Promise resolved [A]']); }); @@ -2094,8 +2138,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { await ReactNoop.act(async () => _setShow(true)); expect(Scheduler).toHaveYielded(['Suspend! [A]']); - Scheduler.unstable_advanceTime(100); - await advanceTimers(100); + await resolveText('A'); expect(Scheduler).toHaveYielded(['Promise resolved [A]']); }); @@ -2829,6 +2872,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { foo.setState({suspend: false}); }); + expect(Scheduler).toHaveYielded(['Foo']); expect(root).toMatchRenderedOutput(); }); @@ -2944,4 +2988,276 @@ 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; + function Child() { + const [text, _setText] = useState('A'); + setText = _setText; + return ; + } + + await resolveText('A'); + await ReactNoop.act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['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 the tree. + ReactNoop.discreteUpdates(() => { + setText('B'); + }); + // Update to a value that has already resolved + await resolveText('C'); + setText('C'); + }); + 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. + 'C', + ]); + 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; + function Child() { + const [text, _setText] = useState('A'); + setText = _setText; + return ; + } + + await resolveText('A'); + await ReactNoop.act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['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'); + + await resolveText('C'); + Scheduler.unstable_runWithPriority( + Scheduler.unstable_IdlePriority, + () => { + setText('C'); + }, + ); + }); + expect(Scheduler).toHaveYielded([ + // First we attempt the high pri update. It suspends. + 'Suspend! [B]', + 'Loading...', + ]); + + // Commit the placeholder to unblock the Idle update. + await advanceTimers(250); + expect(root).toMatchRenderedOutput( + <> +