From 60f190a55948a7512d4e2a336f03b45fd38d6a80 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 26 Jan 2024 12:10:33 -0500 Subject: [PATCH] Capture React.startTransition errors and pass to reportError (#28111) 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. --- .../react-reconciler/src/ReactFiberHooks.js | 5 +- .../src/ReactFiberTransition.js | 14 +--- .../src/__tests__/ReactAsyncActions-test.js | 31 ++++++++ packages/react/src/ReactStartTransition.js | 78 +++++++++++++++---- 4 files changed, 98 insertions(+), 30 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index dd3cf8c273..3cdfae52c2 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -1983,8 +1983,6 @@ function runFormStateAction( } try { const returnValue = action(prevState, payload); - notifyTransitionCallbacks(currentTransition, returnValue); - if ( returnValue !== null && typeof returnValue === 'object' && @@ -1992,6 +1990,7 @@ function runFormStateAction( typeof returnValue.then === 'function' ) { const thenable = ((returnValue: any): Thenable>); + 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( 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( typeof returnValue.then === 'function' ) { const thenable = ((returnValue: any): Thenable); + notifyTransitionCallbacks(currentTransition, thenable); // Create a thenable that resolves to `finishedState` once the async // action has completed. const thenableForFinishedState = chainThenableValue( diff --git a/packages/react-reconciler/src/ReactFiberTransition.js b/packages/react-reconciler/src/ReactFiberTransition.js index d420f67dcf..6179e9daf5 100644 --- a/packages/react-reconciler/src/ReactFiberTransition.js +++ b/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, ): void { - if ( - enableAsyncActions && - returnValue !== null && - typeof returnValue === 'object' && - typeof returnValue.then === 'function' - ) { + if (enableAsyncActions) { // This is an async action. - const thenable: Thenable = (returnValue: any); entangleAsyncAction(transition, thenable); } } diff --git a/packages/react-reconciler/src/__tests__/ReactAsyncActions-test.js b/packages/react-reconciler/src/__tests__/ReactAsyncActions-test.js index d5003fc49b..0a29be2b6f 100644 --- a/packages/react-reconciler/src/__tests__/ReactAsyncActions-test.js +++ b/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,31 @@ describe('ReactAsyncActions', () => { assertLog(['Async action ended', 'Updated']); expect(root).toMatchRenderedOutput(Updated); }); + + test('React.startTransition captures async errors and passes them to reportError', async () => { + // NOTE: This is gated here instead of using the pragma because the failure + // happens asynchronously and the `gate` runtime doesn't capture it. + if (gate(flags => flags.enableAsyncActions)) { + await act(() => { + 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']); + }); }); diff --git a/packages/react/src/ReactStartTransition.js b/packages/react/src/ReactStartTransition.js index ab956a2d2c..ebdc0d7718 100644 --- a/packages/react/src/ReactStartTransition.js +++ b/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 (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; + } + } +} - 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 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); + };