React.startTransition error handling

To make React.startTransition more consistent with the hook form of
startTransition, we capture errors thrown by the scope function and
pass them to the global reportError function. (This is also what we
do as a default for onRecoverableError.)

This is a breaking change because it means that errors inside of
startTransition will no longer bubble up to the caller. You can still
catch the error by putting a try/catch block inside of the scope
function itself.

We do the same for async actions to prevent "unhandled promise
rejection" warnings.

The motivation is to avoid a refactor hazard when changing from a sync
to an async action, or from useTransition to startTransition.

Author: acdlite

packages/react-reconciler/src/ReactFiberHooks.js

@@ -1983,15 +1983,14 @@ function runFormStateAction<S, P>(
   }
   try {
     const returnValue = action(prevState, payload);
-    notifyTransitionCallbacks(currentTransition, returnValue);
-
     if (
       returnValue !== null &&
       typeof returnValue === 'object' &&
       // $FlowFixMe[method-unbinding]
       typeof returnValue.then === 'function'
     ) {
       const thenable = ((returnValue: any): Thenable<Awaited<S>>);
+      notifyTransitionCallbacks(currentTransition, thenable);
 
       // Attach a listener to read the return state of the action. As soon as
       // this resolves, we can run the next action in the sequence.
@@ -2854,7 +2853,6 @@ function startTransition<S>(
   try {
     if (enableAsyncActions) {
       const returnValue = callback();
-      notifyTransitionCallbacks(currentTransition, returnValue);
 
       // Check if we're inside an async action scope. If so, we'll entangle
       // this new action with the existing scope.
@@ -2870,6 +2868,7 @@ function startTransition<S>(
         typeof returnValue.then === 'function'
       ) {
         const thenable = ((returnValue: any): Thenable<mixed>);
+        notifyTransitionCallbacks(currentTransition, thenable);
         // Create a thenable that resolves to `finishedState` once the async
         // action has completed.
         const thenableForFinishedState = chainThenableValue(

packages/react-reconciler/src/ReactFiberTransition.js

@@ -45,23 +45,17 @@ export function requestCurrentTransition(): BatchConfigTransition | null {
   if (transition !== null) {
     // Whenever a transition update is scheduled, register a callback on the
     // transition object so we can get the return value of the scope function.
-    transition._callbacks.add(handleTransitionScopeResult);
+    transition._callbacks.add(handleAsyncAction);
   }
   return transition;
 }
 
-function handleTransitionScopeResult(
+function handleAsyncAction(
   transition: BatchConfigTransition,
-  returnValue: mixed,
+  thenable: Thenable<mixed>,
 ): void {
-  if (
-    enableAsyncActions &&
-    returnValue !== null &&
-    typeof returnValue === 'object' &&
-    typeof returnValue.then === 'function'
-  ) {
+  if (enableAsyncActions) {
     // This is an async action.
-    const thenable: Thenable<mixed> = (returnValue: any);
     entangleAsyncAction(transition, thenable);
   }
 }

packages/react-reconciler/src/__tests__/ReactAsyncActions-test.js

@@ -12,6 +12,10 @@ describe('ReactAsyncActions', () => {
   beforeEach(() => {
     jest.resetModules();
 
+    global.reportError = error => {
+      Scheduler.log('reportError: ' + error.message);
+    };
+
     React = require('react');
     ReactNoop = require('react-noop-renderer');
     Scheduler = require('scheduler');
@@ -1726,4 +1730,29 @@ describe('ReactAsyncActions', () => {
     assertLog(['Async action ended', 'Updated']);
     expect(root).toMatchRenderedOutput(<span>Updated</span>);
   });
+
+  test('React.startTransition captures async errors and passes them to reportError', async () => {
+    if (gate(flags => flags.enableAsyncActions)) {
+      await act(async () => {
+        await React.startTransition(async () => {
+          throw new Error('Oops');
+        });
+      });
+      assertLog(['reportError: Oops']);
+    }
+  });
+
+  // @gate enableAsyncActions
+  test('React.startTransition captures sync errors and passes them to reportError', async () => {
+    await act(() => {
+      try {
+        React.startTransition(() => {
+          throw new Error('Oops');
+        });
+      } catch (e) {
+        throw new Error('Should not be reachable.');
+      }
+    });
+    assertLog(['reportError: Oops']);
+  });
 });

packages/react/src/ReactStartTransition.js

@@ -10,7 +10,10 @@ import type {BatchConfigTransition} from 'react-reconciler/src/ReactFiberTracing
 import type {StartTransitionOptions} from 'shared/ReactTypes';
 
 import ReactCurrentBatchConfig from './ReactCurrentBatchConfig';
-import {enableTransitionTracing} from 'shared/ReactFeatureFlags';
+import {
+  enableAsyncActions,
+  enableTransitionTracing,
+} from 'shared/ReactFeatureFlags';
 
 export function startTransition(
   scope: () => void,
@@ -39,24 +42,65 @@ export function startTransition(
     }
   }
 
-  try {
-    const returnValue = scope();
-    callbacks.forEach(callback => callback(currentTransition, returnValue));
-  } finally {
-    ReactCurrentBatchConfig.transition = prevTransition;
-
-    if (__DEV__) {
-      if (prevTransition === null && currentTransition._updatedFibers) {
-        const updatedFibersCount = currentTransition._updatedFibers.size;
-        currentTransition._updatedFibers.clear();
-        if (updatedFibersCount > 10) {
-          console.warn(
-            'Detected a large number of updates inside startTransition. ' +
-              'If this is due to a subscription please re-write it to use React provided hooks. ' +
-              'Otherwise concurrent mode guarantees are off the table.',
-          );
-        }
+  if (enableAsyncActions) {
+    try {
+      const returnValue = scope();
+      if (
+        typeof returnValue === 'object' &&
+        returnValue !== null &&
+        typeof returnValue.then === 'function'
+      ) {
+        callbacks.forEach(callback => callback(currentTransition, returnValue));
+        returnValue.then(noop, onError);
+      }
+    } catch (error) {
+      onError(error);
+    } finally {
+      warnAboutTransitionSubscriptions(prevTransition, currentTransition);
+      ReactCurrentBatchConfig.transition = prevTransition;
+    }
+  } else {
+    // When async actions are not enabled, startTransition does not
+    // capture errors.
+    try {
+      scope();
+    } finally {
+      warnAboutTransitionSubscriptions(prevTransition, currentTransition);
+      ReactCurrentBatchConfig.transition = prevTransition;
+    }
+  }
+}
+
+function warnAboutTransitionSubscriptions(
+  prevTransition: BatchConfigTransition | null,
+  currentTransition: BatchConfigTransition,
+) {
+  if (__DEV__) {
+    if (prevTransition === null && currentTransition._updatedFibers) {
+      const updatedFibersCount = currentTransition._updatedFibers.size;
+      currentTransition._updatedFibers.clear();
+      if (updatedFibersCount > 10) {
+        console.warn(
+          'Detected a large number of updates inside startTransition. ' +
+            'If this is due to a subscription please re-write it to use React provided hooks. ' +
+            'Otherwise concurrent mode guarantees are off the table.',
+        );
       }
     }
   }
 }
+
+function noop() {}
+
+// Use reportError, if it exists. Otherwise console.error. This is the same as
+// the default for onRecoverableError.
+const onError =
+  typeof reportError === 'function'
+    ? // In modern browsers, reportError will dispatch an error event,
+      // emulating an uncaught JavaScript error.
+      reportError
+    : (error: mixed) => {
+        // In older browsers and test environments, fallback to console.error.
+        // eslint-disable-next-line react-internal/no-production-logging
+        console['error'](error);
+      };