From 039a5990a1ca39ba1858a775d2ab163f98468bb9 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 26 Jan 2024 11:12:13 -0500 Subject: [PATCH] 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. --- .../react-reconciler/src/ReactFiberHooks.js | 5 +- .../src/ReactFiberTransition.js | 14 +--- .../src/__tests__/ReactAsyncActions-test.js | 31 +++++++ packages/react/src/ReactStartTransition.js | 80 ++++++++++++++----- 4 files changed, 99 insertions(+), 31 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index dd3cf8c273d3a..3cdfae52c25db 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 d420f67dcfe76..6179e9daf5105 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 d5003fc49b066..0a29be2b6fec9 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 ab956a2d2cb38..ebdc0d7718a66 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 (__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); + };