From 6e312cf55b5e2aeeec4e93b1dffa10a1299ff617 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 20 Mar 2020 11:02:55 -0700 Subject: [PATCH 1/6] 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. --- ...tSuspenseWithNoopRenderer-test.internal.js | 140 ++++++++++++------ 1 file changed, 91 insertions(+), 49 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 7107a87c3efe..89a860013d21 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([ @@ -1495,9 +1543,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 +1628,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 +1686,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { , ); - Scheduler.unstable_advanceTime(500); - await advanceTimers(500); + await resolveText('B'); expect(Scheduler).toHaveYielded(['Promise resolved [B]']); @@ -1690,8 +1735,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'Effect [Loading...]', ]); - Scheduler.unstable_advanceTime(500); - await advanceTimers(500); + await resolveText('B2'); expect(Scheduler).toHaveYielded(['Promise resolved [B2]']); @@ -2068,8 +2112,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 +2137,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]']); }); From 2eab95a21788685c907809ed830c86a332f92ab6 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 24 Mar 2020 22:04:04 -0700 Subject: [PATCH 2/6] 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. --- .../src/ReactFiberBeginWork.js | 64 ++++++++- ...eactHooksWithNoopRenderer-test.internal.js | 14 +- .../__tests__/ReactSuspense-test.internal.js | 9 +- ...tSuspenseWithNoopRenderer-test.internal.js | 129 ++++++++++++++++++ 4 files changed, 210 insertions(+), 6 deletions(-) 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 89a860013d21..15b22fa94a16 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -2871,6 +2871,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { foo.setState({suspend: false}); }); + expect(Scheduler).toHaveYielded(['Foo']); expect(root).toMatchRenderedOutput(); }); @@ -2986,4 +2987,132 @@ 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...', + + // 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( + <> +