From c4c9f086eb9b61a36d9d96a847374ea65147b6cb Mon Sep 17 00:00:00 2001 From: lunaruan Date: Fri, 2 Aug 2019 13:54:11 -0700 Subject: [PATCH] BugFix: Suspense priority warning firing when not supposed to (#16256) Previously, the suspense priority warning was fired even if the Root wasn't suspended. Changed the warning to fire only when the root is suspended. Also refactored the suspense priority warning so it's easier to read. --- .../src/ReactFiberWorkLoop.js | 80 +++----- .../__tests__/ReactSuspense-test.internal.js | 11 -- ...tSuspenseWithNoopRenderer-test.internal.js | 172 +++++++----------- .../__tests__/ReactProfiler-test.internal.js | 12 -- 4 files changed, 88 insertions(+), 187 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 76698b67185b2..2971a4764a772 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -802,7 +802,6 @@ function prepareFreshStack(root, expirationTime) { if (__DEV__) { ReactStrictModeWarnings.discardPendingWarnings(); - componentsThatSuspendedAtHighPri = null; componentsThatTriggeredHighPriSuspend = null; } } @@ -990,8 +989,6 @@ function renderRoot( // Set this to null to indicate there's no in-progress render. workInProgressRoot = null; - flushSuspensePriorityWarningInDEV(); - switch (workInProgressRootExitStatus) { case RootIncomplete: { invariant(false, 'Should have a work-in-progress.'); @@ -1022,6 +1019,8 @@ function renderRoot( return commitRoot.bind(null, root); } case RootSuspended: { + flushSuspensePriorityWarningInDEV(); + // We have an acceptable loading state. We need to figure out if we should // immediately commit it or wait a bit. @@ -1076,6 +1075,8 @@ function renderRoot( return commitRoot.bind(null, root); } case RootSuspendedWithDelay: { + flushSuspensePriorityWarningInDEV(); + if ( !isSync && // do not delay if we're inside an act() scope @@ -2610,7 +2611,6 @@ export function warnIfUnmockedScheduler(fiber: Fiber) { } } -let componentsThatSuspendedAtHighPri = null; let componentsThatTriggeredHighPriSuspend = null; export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) { if (__DEV__) { @@ -2697,70 +2697,34 @@ export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) { } workInProgressNode = workInProgressNode.return; } - - // Add the component name to a set. - const componentName = getComponentName(sourceFiber.type); - if (componentsThatSuspendedAtHighPri === null) { - componentsThatSuspendedAtHighPri = new Set([componentName]); - } else { - componentsThatSuspendedAtHighPri.add(componentName); - } } } } function flushSuspensePriorityWarningInDEV() { if (__DEV__) { - if (componentsThatSuspendedAtHighPri !== null) { + if (componentsThatTriggeredHighPriSuspend !== null) { const componentNames = []; - componentsThatSuspendedAtHighPri.forEach(name => { - componentNames.push(name); - }); - componentsThatSuspendedAtHighPri = null; - - const componentsThatTriggeredSuspendNames = []; - if (componentsThatTriggeredHighPriSuspend !== null) { - componentsThatTriggeredHighPriSuspend.forEach(name => - componentsThatTriggeredSuspendNames.push(name), - ); - } - + componentsThatTriggeredHighPriSuspend.forEach(name => + componentNames.push(name), + ); componentsThatTriggeredHighPriSuspend = null; - const componentNamesString = componentNames.sort().join(', '); - let componentThatTriggeredSuspenseError = ''; - if (componentsThatTriggeredSuspendNames.length > 0) { - componentThatTriggeredSuspenseError = - 'The following components triggered a user-blocking update:' + - '\n\n' + - ' ' + - componentsThatTriggeredSuspendNames.sort().join(', ') + - '\n\n' + - 'that was then suspended by:' + - '\n\n' + - ' ' + - componentNamesString; - } else { - componentThatTriggeredSuspenseError = - 'A user-blocking update was suspended by:' + - '\n\n' + - ' ' + - componentNamesString; + if (componentNames.length > 0) { + warningWithoutStack( + false, + '%s triggered a user-blocking update that suspended.' + + '\n\n' + + 'The fix is to split the update into multiple parts: a user-blocking ' + + 'update to provide immediate feedback, and another update that ' + + 'triggers the bulk of the changes.' + + '\n\n' + + 'Refer to the documentation for useSuspenseTransition to learn how ' + + 'to implement this pattern.', + // TODO: Add link to React docs with more information, once it exists + componentNames.sort().join(', '), + ); } - - warningWithoutStack( - false, - '%s' + - '\n\n' + - 'The fix is to split the update into multiple parts: a user-blocking ' + - 'update to provide immediate feedback, and another update that ' + - 'triggers the bulk of the changes.' + - '\n\n' + - 'Refer to the documentation for useSuspenseTransition to learn how ' + - 'to implement this pattern.', - // TODO: Add link to React docs with more information, once it exists - componentThatTriggeredSuspenseError, - ); } } } diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 5050c2936ffe1..32e8336d7c6b1 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -327,7 +327,6 @@ describe('ReactSuspense', () => { }); it('throws if tree suspends and none of the Suspense ancestors have a fallback', () => { - spyOnDev(console, 'error'); ReactTestRenderer.create( @@ -341,16 +340,6 @@ describe('ReactSuspense', () => { 'AsyncText suspended while rendering, but no fallback UI was specified.', ); expect(Scheduler).toHaveYielded(['Suspend! [Hi]', 'Suspend! [Hi]']); - if (__DEV__) { - expect(console.error).toHaveBeenCalledTimes(2); - expect(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: %s\n\nThe fix is to split the update', - ); - expect(console.error.calls.argsFor(0)[1]).toContain( - 'A user-blocking update was suspended by:', - ); - expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText'); - } }); describe('outside concurrent mode', () => { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index c95e058bb6af0..b2a2a7c5393e3 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -489,7 +489,6 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); it('tries rendering a lower priority pending update even if a higher priority one suspends', async () => { - spyOnDev(console, 'error'); function App(props) { if (props.hide) { return ; @@ -517,16 +516,6 @@ describe('ReactSuspenseWithNoopRenderer', () => { '(empty)', ]); expect(ReactNoop.getChildren()).toEqual([span('(empty)')]); - if (__DEV__) { - expect(console.error).toHaveBeenCalledTimes(1); - expect(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: %s\n\nThe fix is to split the update', - ); - expect(console.error.calls.argsFor(0)[1]).toContain( - 'A user-blocking update was suspended by:', - ); - expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText'); - } }); it('forces an expiration after an update times out', async () => { @@ -671,22 +660,9 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toHaveYielded(['Promise resolved [Async]']); expect(Scheduler).toFlushAndYield(['Async']); expect(ReactNoop.getChildren()).toEqual([span('Async'), span('Sync')]); - - if (__DEV__) { - expect(console.error).toHaveBeenCalledTimes(1); - expect(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: %s\n\nThe fix is to split the update', - ); - expect(console.error.calls.argsFor(0)[1]).toContain( - 'A user-blocking update was suspended by:', - ); - expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText'); - } }); it('suspending inside an expired expiration boundary will bubble to the next one', async () => { - spyOnDev(console, 'error'); - ReactNoop.flushSync(() => ReactNoop.render( @@ -707,17 +683,6 @@ describe('ReactSuspenseWithNoopRenderer', () => { ]); // The tree commits synchronously expect(ReactNoop.getChildren()).toEqual([span('Loading (outer)...')]); - - if (__DEV__) { - expect(console.error).toHaveBeenCalledTimes(1); - expect(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: %s\n\nThe fix is to split the update', - ); - expect(console.error.calls.argsFor(0)[1]).toContain( - 'A user-blocking update was suspended by:', - ); - expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText'); - } }); it('expires early by default', async () => { @@ -795,21 +760,10 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); it('throws a helpful error when an update is suspends without a placeholder', () => { - spyOnDev(console, 'error'); ReactNoop.render(); expect(Scheduler).toFlushAndThrow( 'AsyncText suspended while rendering, but no fallback UI was specified.', ); - if (__DEV__) { - expect(console.error).toHaveBeenCalledTimes(2); - expect(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: %s\n\nThe fix is to split the update', - ); - expect(console.error.calls.argsFor(0)[1]).toContain( - 'A user-blocking update was suspended by:', - ); - expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText'); - } }); it('a Suspense component correctly handles more than one suspended child', async () => { @@ -1637,18 +1591,11 @@ describe('ReactSuspenseWithNoopRenderer', () => { Scheduler.unstable_advanceTime(100); await advanceTimers(100); - expect(() => { - expect(Scheduler).toFlushAndYield([ - // A suspends - 'Suspend! [A]', - 'Loading...', - ]); - }).toWarnDev( - 'Warning: A user-blocking update was suspended by:' + - '\n\n' + - ' AsyncText', - {withoutStack: true}, - ); + expect(Scheduler).toFlushAndYield([ + // A suspends + 'Suspend! [A]', + 'Loading...', + ]); // We're now suspended and we haven't shown anything yet. expect(ReactNoop.getChildren()).toEqual([]); @@ -1689,13 +1636,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { ); }); }).toWarnDev( - 'Warning: The following components triggered a user-blocking update:' + - '\n\n' + - ' App' + - '\n\n' + - 'that was then suspended by:' + - '\n\n' + - ' AsyncText', + 'Warning: App triggered a user-blocking update that suspended.' + '\n\n', {withoutStack: true}, ); }); @@ -1727,59 +1668,78 @@ describe('ReactSuspenseWithNoopRenderer', () => { ); }); }).toWarnDev( - 'Warning: The following components triggered a user-blocking update:' + - '\n\n' + - ' App' + - '\n\n' + - 'that was then suspended by:' + - '\n\n' + - ' AsyncText', + 'Warning: App triggered a user-blocking update that suspended.' + '\n\n', {withoutStack: true}, ); }); - it('warns when suspending inside discrete update', async () => { - function A() { - Scheduler.unstable_yieldValue('A'); - TextResource.read(['A', 1000]); - return 'A'; - } + it('does not warn about wrong Suspense priority if no new fallbacks are shown', async () => { + let showB; + class App extends React.Component { + state = {showB: false}; - function B() { - return 'B'; + render() { + showB = () => this.setState({showB: true}); + return ( + + {} + {this.state.showB && } + + ); + } } - function C() { - TextResource.read(['C', 1000]); - return 'C'; - } + await ReactNoop.act(async () => { + ReactNoop.render(); + }); - function App() { - return ( - - - - - - - - + expect(Scheduler).toHaveYielded(['Suspend! [A]']); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); + + ReactNoop.act(() => { + Scheduler.unstable_runWithPriority( + Scheduler.unstable_UserBlockingPriority, + () => showB(), ); - } + }); - ReactNoop.discreteUpdates(() => ReactNoop.render()); - expect(Scheduler).toFlushAndYieldThrough(['A']); + expect(Scheduler).toHaveYielded(['Suspend! [A]', 'Suspend! [B]']); + }); - // Warning is not flushed until the commit phase + it( + 'warns when component that triggered user-blocking update is between Suspense boundary ' + + 'and component that suspended', + async () => { + let _setShow; + function A() { + const [show, setShow] = React.useState(false); + _setShow = setShow; + return show && ; + } + function App() { + return ( + + + + ); + } + await ReactNoop.act(async () => { + ReactNoop.render(); + }); - // Timeout and commit the fallback - expect(() => { - Scheduler.unstable_flushAll(); - }).toWarnDev( - 'Warning: A user-blocking update was suspended by:' + '\n\n' + ' A, C', - {withoutStack: true}, - ); - }); + expect(() => { + ReactNoop.act(() => { + Scheduler.unstable_runWithPriority( + Scheduler.unstable_UserBlockingPriority, + () => _setShow(true), + ); + }); + }).toWarnDev( + 'Warning: A triggered a user-blocking update that suspended.' + '\n\n', + {withoutStack: true}, + ); + }, + ); it('normal priority updates suspending do not warn for class components', async () => { let show; diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index c1a99452946f0..9edad199ef4b0 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -2626,7 +2626,6 @@ describe('Profiler', () => { }); it('handles high-pri renderers between suspended and resolved (async) trees', async () => { - spyOnDev(console, 'error'); // Set up an initial shell. We need to set this up before the test sceanrio // because we want initial render to suspend on navigation to the initial state. let renderer = ReactTestRenderer.create( @@ -2729,17 +2728,6 @@ describe('Profiler', () => { expect( onInteractionScheduledWorkCompleted.mock.calls[1][0], ).toMatchInteraction(highPriUpdateInteraction); - - if (__DEV__) { - expect(console.error).toHaveBeenCalledTimes(1); - expect(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: %s\n\nThe fix is to split the update', - ); - expect(console.error.calls.argsFor(0)[1]).toContain( - 'A user-blocking update was suspended by:', - ); - expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText'); - } }); }); });