From 9d67847f7b41e408016cfa16450323b3fee8bc56 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 26 Mar 2020 11:31:40 -0700 Subject: [PATCH] [Bugfix] Dropped updates inside a suspended tree (#18384) * Minor test refactor: `resolveText` Adds a `resolveText` method as an alternative to using timers. Also removes dependency on react-cache (for just this one test file; can do the others later). Timer option is still there if you provide a `ms` prop. * Bugfix: Dropped updates in suspended tree When there are multiple updates at different priority levels inside a suspended subtree, all but the highest priority one is dropped after the highest one suspends. We do have tests that cover this for updates that originate outside of the Suspense boundary, but not for updates that originate inside. I'm surprised it's taken us this long to find this issue, but it makes sense in that transition updates usually originate outside the boundary or "seam" of the part of the UI that is transitioning. * Bugfix: Suspense fragment skipped by setState Fixes a bug where updates inside a suspended tree are dropped because the fragment fiber we insert to wrap the hidden children is not part of the return path, so it doesn't get marked during setState. As a workaround, I recompute `childExpirationTime` right before deciding to bail out by bubbling it up from the next level of children. This is something we should consider addressing when we refactor the Fiber data structure. * Add back `lastPendingTime` field This reverts commit 9a541139dfe36e8b9b02b1c6585889e2abf97389. I want to use this so we can check if there might be any lower priority updates in a suspended tree. We can remove it again during the expiration times refactor. * Use `lastPendingTime` instead of Idle We don't currently have an mechanism to check if there are lower priority updates in a subtree, but we can check if there are any in the whole root. This still isn't perfect but it's better than using Idle, which frequently leads to redundant re-renders. When we refactor `expirationTime` to be a bitmask, this will no longer be necessary because we'll know exactly which "task bits" remain. * Add a test for updating the fallback --- .../src/ReactFiberBeginWork.js | 92 +++- .../react-reconciler/src/ReactFiberRoot.js | 12 + .../ReactSuspenseList-test.internal.js | 8 +- ...tSuspenseWithNoopRenderer-test.internal.js | 414 +++++++++++++++--- 4 files changed, 473 insertions(+), 53 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 2231dc5ae5b61..014782b2fd6dc 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 d035d31c764f0..544dcd0764144 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 12a6d03a2313e..7928ca491b082 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 7107a87c3efe6..55793fb6c3b34 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( + <> +