diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 56bdb8132d49..f748228f8eb0 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -698,6 +698,7 @@ function prepareFreshStack(root, expirationTime) { if (__DEV__) { ReactStrictModeWarnings.discardPendingWarnings(); + componentsWithSuspendedDiscreteUpdates = null; } } @@ -1225,6 +1226,7 @@ function commitRoot(root, expirationTime) { function commitRootImpl(root, expirationTime) { flushPassiveEffects(); flushRenderPhaseStrictModeWarningsInDEV(); + flushSuspensePriorityWarningInDEV(); invariant( workPhase !== RenderPhase && workPhase !== CommitPhase, @@ -2114,6 +2116,76 @@ function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { export const warnIfNotCurrentlyActingUpdatesInDev = warnIfNotCurrentlyActingUpdatesInDEV; +let componentsWithSuspendedDiscreteUpdates = null; +export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) { + if (__DEV__) { + 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) + ) { + // Add the component name to a set. + const componentName = getComponentName(sourceFiber.type); + if (componentsWithSuspendedDiscreteUpdates === null) { + componentsWithSuspendedDiscreteUpdates = new Set([componentName]); + } else { + componentsWithSuspendedDiscreteUpdates.add(componentName); + } + } + } +} + +function flushSuspensePriorityWarningInDEV() { + if (__DEV__) { + if (componentsWithSuspendedDiscreteUpdates !== null) { + const componentNames = []; + componentsWithSuspendedDiscreteUpdates.forEach(name => { + componentNames.push(name); + }); + componentsWithSuspendedDiscreteUpdates = 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`. + warningWithoutStack( + false, + 'The following components suspended during a user-blocking update: %s' + + '\n\n' + + 'Updates triggered by user interactions (e.g. click events) are ' + + 'considered user-blocking by default. They should not suspend. ' + + 'Updates that can afford to take a bit longer should be wrapped ' + + 'with `Scheduler.next` (or an equivalent abstraction). This ' + + 'typically includes any update that shows new content, like ' + + 'a navigation.' + + '\n\n' + + 'Generally, you should split user interactions into at least two ' + + 'seprate updates: a user-blocking update to provide immediate ' + + 'feedback, and another update to perform the actual change.', + // TODO: Add link to React docs with more information, once it exists + componentNames.sort().join(', '), + ); + } + } +} + function computeThreadID(root, expirationTime) { // Interaction threads are unique per root and expiration time. return expirationTime * 1000 + root.interactionThreadID; diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index c6c827673303..79525c96ad52 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -68,6 +68,7 @@ import { isAlreadyFailedLegacyErrorBoundary, pingSuspendedRoot, resolveRetryThenable, + checkForWrongSuspensePriorityInDEV, } from './ReactFiberScheduler'; import invariant from 'shared/invariant'; @@ -203,6 +204,8 @@ function throwException( // This is a thenable. const thenable: Thenable = (value: any); + checkForWrongSuspensePriorityInDEV(sourceFiber); + // Schedule the nearest Suspense to re-render the timed out view. let workInProgress = returnFiber; do { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 10764e607cc1..34a72083e87c 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -1715,7 +1715,14 @@ describe('ReactSuspenseWithNoopRenderer', () => { // Flush some of the time Scheduler.advanceTime(500); - await advanceTimers(500); + expect(() => { + jest.advanceTimersByTime(500); + }).toWarnDev( + 'The following components suspended during a user-blocking ' + + 'update: AsyncText', + {withoutStack: true}, + ); + // 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 @@ -1723,4 +1730,46 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toFlushWithoutYielding(); expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); }); + + it('warns when suspending inside discrete update', async () => { + function A() { + TextResource.read(['A', 1000]); + return 'A'; + } + + function B() { + return 'B'; + } + + function C() { + TextResource.read(['C', 1000]); + return 'C'; + } + + function App() { + return ( + + + + + + + + + ); + } + + ReactNoop.interactiveUpdates(() => ReactNoop.render()); + Scheduler.flushAll(); + + // Warning is not flushed until the commit phase + + // Timeout and commit the fallback + expect(() => { + jest.advanceTimersByTime(1000); + }).toWarnDev( + 'The following components suspended during a user-blocking update: A, C', + {withoutStack: true}, + ); + }); });