diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 451b8a9cda..7a98440b59 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -13,6 +13,7 @@ import type {Fiber} from './ReactFiber'; import type {ExpirationTime} from './ReactFiberExpirationTime'; import type {HookEffectTag} from './ReactHookEffectTags'; import type {SuspenseConfig} from './ReactFiberSuspenseConfig'; +import type {ReactPriorityLevel} from './SchedulerWithReactIntegration'; import ReactSharedInternals from 'shared/ReactSharedInternals'; @@ -48,6 +49,7 @@ import is from 'shared/objectIs'; import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork'; import {revertPassiveEffectsChange} from 'shared/ReactFeatureFlags'; import {requestCurrentSuspenseConfig} from './ReactFiberSuspenseConfig'; +import {getCurrentPriorityLevel} from './SchedulerWithReactIntegration'; const {ReactCurrentDispatcher} = ReactSharedInternals; @@ -96,6 +98,8 @@ type Update = { eagerReducer: ((S, A) => S) | null, eagerState: S | null, next: Update | null, + + priority?: ReactPriorityLevel, }; type UpdateQueue = { @@ -1140,6 +1144,9 @@ function dispatchAction( eagerState: null, next: null, }; + if (__DEV__) { + update.priority = getCurrentPriorityLevel(); + } if (renderPhaseUpdates === null) { renderPhaseUpdates = new Map(); } @@ -1176,6 +1183,10 @@ function dispatchAction( next: null, }; + if (__DEV__) { + update.priority = getCurrentPriorityLevel(); + } + // Append the update to the end of the list. const last = queue.last; if (last === null) { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 510f31c2b5..966573139c 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -795,7 +795,8 @@ function prepareFreshStack(root, expirationTime) { if (__DEV__) { ReactStrictModeWarnings.discardPendingWarnings(); - componentsWithSuspendedDiscreteUpdates = null; + componentsThatSuspendedAtHighPri = null; + componentsThatTriggeredHighPriSuspend = null; } } @@ -982,6 +983,8 @@ 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.'); @@ -1491,7 +1494,6 @@ function commitRoot(root) { function commitRootImpl(root) { flushPassiveEffects(); flushRenderPhaseStrictModeWarningsInDEV(); - flushSuspensePriorityWarningInDEV(); invariant( (executionContext & (RenderContext | CommitContext)) === NoContext, @@ -2520,37 +2522,100 @@ function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { export const warnIfNotCurrentlyActingUpdatesInDev = warnIfNotCurrentlyActingUpdatesInDEV; -let componentsWithSuspendedDiscreteUpdates = null; +let componentsThatSuspendedAtHighPri = null; +let componentsThatTriggeredHighPriSuspend = null; export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) { if (__DEV__) { + const currentPriorityLevel = getCurrentPriorityLevel(); if ( (sourceFiber.mode & ConcurrentMode) !== NoEffect && - // Check if we're currently rendering a discrete update. Ideally, all we - // would need to do is check the current priority level. But we currently - // have no rigorous way to distinguish work that was scheduled at user- - // blocking priority from work that expired a bit and was "upgraded" to - // a higher priority. That's because we don't schedule separate callbacks - // for every level, only the highest priority level per root. The priority - // of subsequent levels is inferred from the expiration time, but this is - // an imprecise heuristic. - // - // However, we do store the last discrete pending update per root. So we - // can reliably compare to that one. (If we broaden this warning to include - // high pri updates that aren't discrete, then this won't be sufficient.) - // - // My rationale is that it's better for this warning to have false - // negatives than false positives. - rootsWithPendingDiscreteUpdates !== null && - workInProgressRoot !== null && - renderExpirationTime === - rootsWithPendingDiscreteUpdates.get(workInProgressRoot) + (currentPriorityLevel === UserBlockingPriority || + currentPriorityLevel === ImmediatePriority) ) { + let workInProgressNode = sourceFiber; + while (workInProgressNode !== null) { + // Add the component that triggered the suspense + const current = workInProgressNode.alternate; + if (current !== null) { + // TODO: warn component that triggers the high priority + // suspend is the HostRoot + switch (workInProgressNode.tag) { + case ClassComponent: + // Loop through the component's update queue and see whether the component + // has triggered any high priority updates + const updateQueue = current.updateQueue; + if (updateQueue !== null) { + let update = updateQueue.firstUpdate; + while (update !== null) { + const priorityLevel = update.priority; + if ( + priorityLevel === UserBlockingPriority || + priorityLevel === ImmediatePriority + ) { + if (componentsThatTriggeredHighPriSuspend === null) { + componentsThatTriggeredHighPriSuspend = new Set([ + getComponentName(workInProgressNode.type), + ]); + } else { + componentsThatTriggeredHighPriSuspend.add( + getComponentName(workInProgressNode.type), + ); + } + break; + } + update = update.next; + } + } + break; + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: + if ( + workInProgressNode.memoizedState !== null && + workInProgressNode.memoizedState.baseUpdate !== null + ) { + let update = workInProgressNode.memoizedState.baseUpdate; + // Loop through the functional component's memoized state to see whether + // the component has triggered any high pri updates + while (update !== null) { + const priority = update.priority; + if ( + priority === UserBlockingPriority || + priority === ImmediatePriority + ) { + if (componentsThatTriggeredHighPriSuspend === null) { + componentsThatTriggeredHighPriSuspend = new Set([ + getComponentName(workInProgressNode.type), + ]); + } else { + componentsThatTriggeredHighPriSuspend.add( + getComponentName(workInProgressNode.type), + ); + } + break; + } + if ( + update.next === workInProgressNode.memoizedState.baseUpdate + ) { + break; + } + update = update.next; + } + } + break; + default: + break; + } + } + workInProgressNode = workInProgressNode.return; + } + // Add the component name to a set. const componentName = getComponentName(sourceFiber.type); - if (componentsWithSuspendedDiscreteUpdates === null) { - componentsWithSuspendedDiscreteUpdates = new Set([componentName]); + if (componentsThatSuspendedAtHighPri === null) { + componentsThatSuspendedAtHighPri = new Set([componentName]); } else { - componentsWithSuspendedDiscreteUpdates.add(componentName); + componentsThatSuspendedAtHighPri.add(componentName); } } } @@ -2558,20 +2623,32 @@ export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) { function flushSuspensePriorityWarningInDEV() { if (__DEV__) { - if (componentsWithSuspendedDiscreteUpdates !== null) { + if (componentsThatSuspendedAtHighPri !== null) { const componentNames = []; - componentsWithSuspendedDiscreteUpdates.forEach(name => { + componentsThatSuspendedAtHighPri.forEach(name => { componentNames.push(name); }); - componentsWithSuspendedDiscreteUpdates = null; + componentsThatSuspendedAtHighPri = null; - // TODO: A more helpful version of this message could include the names of - // the component that were updated, not the ones that suspended. To do - // that we'd need to track all the components that updated during this - // render, perhaps using the same mechanism as `markRenderEventTime`. + const componentsThatTriggeredSuspendNames = []; + if (componentsThatTriggeredHighPriSuspend !== null) { + componentsThatTriggeredHighPriSuspend.forEach(name => + componentsThatTriggeredSuspendNames.push(name), + ); + } + + componentsThatTriggeredHighPriSuspend = null; + + const componentThatTriggeredSuspenseError = + componentsThatTriggeredSuspendNames.length > 0 + ? '\n' + + 'The components that triggered the update: ' + + componentsThatTriggeredSuspendNames.sort().join(', ') + : ''; warningWithoutStack( false, 'The following components suspended during a user-blocking update: %s' + + '%s' + '\n\n' + 'Updates triggered by user interactions (e.g. click events) are ' + 'considered user-blocking by default. They should not suspend. ' + @@ -2585,6 +2662,7 @@ function flushSuspensePriorityWarningInDEV() { 'feedback, and another update to perform the actual change.', // TODO: Add link to React docs with more information, once it exists componentNames.sort().join(', '), + componentThatTriggeredSuspenseError, ); } } diff --git a/packages/react-reconciler/src/ReactUpdateQueue.js b/packages/react-reconciler/src/ReactUpdateQueue.js index 2005d6aaec..4356d445c1 100644 --- a/packages/react-reconciler/src/ReactUpdateQueue.js +++ b/packages/react-reconciler/src/ReactUpdateQueue.js @@ -87,6 +87,7 @@ import type {Fiber} from './ReactFiber'; import type {ExpirationTime} from './ReactFiberExpirationTime'; import type {SuspenseConfig} from './ReactFiberSuspenseConfig'; +import type {ReactPriorityLevel} from './SchedulerWithReactIntegration'; import {NoWork} from './ReactFiberExpirationTime'; import { @@ -106,6 +107,7 @@ import {markRenderEventTimeAndConfig} from './ReactFiberWorkLoop'; import invariant from 'shared/invariant'; import warningWithoutStack from 'shared/warningWithoutStack'; +import {getCurrentPriorityLevel} from './SchedulerWithReactIntegration'; export type Update = { expirationTime: ExpirationTime, @@ -117,6 +119,9 @@ export type Update = { next: Update | null, nextEffect: Update | null, + + //DEV only + priority?: ReactPriorityLevel, }; export type UpdateQueue = { @@ -197,7 +202,7 @@ export function createUpdate( expirationTime: ExpirationTime, suspenseConfig: null | SuspenseConfig, ): Update<*> { - return { + let update: Update<*> = { expirationTime, suspenseConfig, @@ -208,6 +213,10 @@ export function createUpdate( next: null, nextEffect: null, }; + if (__DEV__) { + update.priority = getCurrentPriorityLevel(); + } + return update; } function appendUpdateToQueue( diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 32e8336d7c..d313de6b59 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -327,6 +327,7 @@ describe('ReactSuspense', () => { }); it('throws if tree suspends and none of the Suspense ancestors have a fallback', () => { + spyOnDev(console, 'error'); ReactTestRenderer.create( @@ -340,6 +341,13 @@ 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: The following components suspended during a user-blocking update: ', + ); + 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 a32b6b2af8..00dc20493c 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -488,6 +488,7 @@ 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 ; @@ -515,6 +516,13 @@ 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: The following components suspended during a user-blocking update: ', + ); + expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText'); + } }); it('forces an expiration after an update times out', async () => { @@ -631,6 +639,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); it('renders an expiration boundary synchronously', async () => { + spyOnDev(console, 'error'); // Synchronously render a tree that suspends ReactNoop.flushSync(() => ReactNoop.render( @@ -658,9 +667,19 @@ 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: The following components suspended during a user-blocking update: ', + ); + 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( @@ -681,6 +700,14 @@ 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: The following components suspended during a user-blocking update: ', + ); + expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText'); + } }); it('expires early by default', async () => { @@ -758,11 +785,18 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); it('throws a helpful error when an update is suspends without a placeholder', () => { - expect(() => { - ReactNoop.flushSync(() => ReactNoop.render()); - }).toThrow( + 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: The following components suspended during a user-blocking update: ', + ); + expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText'); + } }); it('a Suspense component correctly handles more than one suspended child', async () => { @@ -1590,24 +1624,25 @@ describe('ReactSuspenseWithNoopRenderer', () => { Scheduler.unstable_advanceTime(100); await advanceTimers(100); - expect(Scheduler).toFlushAndYield([ - // A suspends - 'Suspend! [A]', - 'Loading...', - ]); - // We're now suspended and we haven't shown anything yet. - expect(ReactNoop.getChildren()).toEqual([]); - - // Flush some of the time - Scheduler.unstable_advanceTime(500); expect(() => { - jest.advanceTimersByTime(500); + expect(Scheduler).toFlushAndYield([ + // A suspends + 'Suspend! [A]', + 'Loading...', + ]); }).toWarnDev( 'The following components suspended during a user-blocking ' + 'update: AsyncText', {withoutStack: true}, ); + // We're now suspended and we haven't shown anything yet. + expect(ReactNoop.getChildren()).toEqual([]); + + // Flush some of the time + Scheduler.unstable_advanceTime(500); + jest.advanceTimersByTime(500); + // We should have already shown the fallback. // When we wrote this test, we inferred the start time of high priority // updates as way earlier in the past. This test ensures that we don't @@ -1616,6 +1651,71 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); }); + it('warns when a low priority update suspends inside a high priority update for functional components', async () => { + let _setShow; + function App() { + let [show, setShow] = React.useState(false); + _setShow = setShow; + return ( + + {show && } + + ); + } + + await ReactNoop.act(async () => { + ReactNoop.render(); + }); + + expect(() => { + ReactNoop.act(() => { + Scheduler.unstable_runWithPriority( + Scheduler.unstable_UserBlockingPriority, + () => _setShow(true), + ); + }); + }).toWarnDev( + 'The following components suspended during a user-blocking update: AsyncText' + + '\n' + + 'The components that triggered the update: App', + {withoutStack: true}, + ); + }); + + it('warns when a low priority update suspends inside a high priority update for class components', async () => { + let show; + class App extends React.Component { + state = {show: false}; + + render() { + show = () => this.setState({show: true}); + return ( + + {this.state.show && } + + ); + } + } + + await ReactNoop.act(async () => { + ReactNoop.render(); + }); + + expect(() => { + ReactNoop.act(() => { + Scheduler.unstable_runWithPriority( + Scheduler.unstable_UserBlockingPriority, + () => show(), + ); + }); + }).toWarnDev( + 'The following components suspended during a user-blocking update: AsyncText' + + '\n' + + 'The components that triggered the update: App', + {withoutStack: true}, + ); + }); + it('warns when suspending inside discrete update', async () => { function A() { Scheduler.unstable_yieldValue('A'); @@ -1654,11 +1754,66 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(() => { Scheduler.unstable_flushAll(); }).toWarnDev( - 'The following components suspended during a user-blocking update: A, C', + 'Warning: The following components suspended during a user-blocking update: A, C', {withoutStack: true}, ); }); + it('normal priority updates suspending do not warn for class components', async () => { + let show; + class App extends React.Component { + state = {show: false}; + + render() { + show = () => this.setState({show: true}); + return ( + + {this.state.show && } + + ); + } + } + + await ReactNoop.act(async () => { + ReactNoop.render(); + }); + + // also make sure lowpriority is okay + await ReactNoop.act(async () => show(true)); + + expect(Scheduler).toHaveYielded(['Suspend! [A]']); + Scheduler.unstable_advanceTime(100); + await advanceTimers(100); + + expect(Scheduler).toHaveYielded(['Promise resolved [A]']); + }); + + it('normal priority updates suspending do not warn for functional components', async () => { + let _setShow; + function App() { + let [show, setShow] = React.useState(false); + _setShow = setShow; + return ( + + {show && } + + ); + } + + await ReactNoop.act(async () => { + ReactNoop.render(); + }); + + // also make sure lowpriority is okay + await ReactNoop.act(async () => _setShow(true)); + + expect(Scheduler).toHaveYielded(['Suspend! [A]']); + Scheduler.unstable_advanceTime(100); + await advanceTimers(100); + + expect(Scheduler).toHaveYielded(['Promise resolved [A]']); + }); + it('shows the parent fallback if the inner fallback should be avoided', async () => { function Foo({showC}) { Scheduler.unstable_yieldValue('Foo'); diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index dd0b2c7212..6323f02049 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -2628,7 +2628,8 @@ describe('Profiler', () => { }); it('handles high-pri renderers between suspended and resolved (async) trees', async () => { - // Set up an initial shell. We need to set this up before the test scenario + 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( {}}> @@ -2730,6 +2731,14 @@ 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: The following components suspended during a user-blocking update: ', + ); + expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText'); + } }); }); });