Warn when suspending at wrong priority (#15492)

* Warn when suspending at wrong priority

Adds a warning when a user-blocking update is suspended.

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.

Potential follow-ups:
- Bikeshed more on the message. I don't like what I landed on that much
but I think it's good enough to start.
- Include the names of the components that updated. (The ideal place to
fire the warning is during the setState call but we don't know if
something will suspend until the next update. Maybe we could be clever
and warn during a subsequent update to the same component?)

* Move Suspense priority check to throwException

Author: acdlite

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;

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 {

packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js

@@ -1715,12 +1715,61 @@ 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
     // use this assumption to add a very long JND.
     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 (
+        <Suspense fallback="Loading...">
+          <A />
+          <B />
+          <C />
+          <C />
+          <C />
+          <C />
+        </Suspense>
+      );
+    }
+
+    ReactNoop.interactiveUpdates(() => ReactNoop.render(<App />));
+    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},
+    );
+  });
 });