diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 0af8817ff98..6ce6f61430a 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -800,7 +800,6 @@ function prepareFreshStack(root, expirationTime) { if (__DEV__) { ReactStrictModeWarnings.discardPendingWarnings(); - componentsThatSuspendedAtHighPri = null; componentsThatTriggeredHighPriSuspend = null; } } @@ -988,8 +987,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.'); @@ -1020,6 +1017,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. @@ -1070,6 +1069,8 @@ function renderRoot( return commitRoot.bind(null, root); } case RootSuspendedWithDelay: { + flushSuspensePriorityWarningInDEV(); + if ( !isSync && // do not delay if we're inside an act() scope @@ -2574,7 +2575,6 @@ export function warnIfUnmockedScheduler(fiber: Fiber) { } } -let componentsThatSuspendedAtHighPri = null; let componentsThatTriggeredHighPriSuspend = null; export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) { if (__DEV__) { @@ -2661,70 +2661,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 5050c2936ff..32e8336d7c6 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 c95e058bb6a..b2a2a7c5393 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 c1a99452946..9edad199ef4 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'); - } }); }); });