Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Capture React.startTransition errors and pass to reportError #28111

Merged
merged 1 commit into from
Jan 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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(
Expand Down
14 changes: 4 additions & 10 deletions packages/react-reconciler/src/ReactFiberTransition.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hoisted this type check into startTransition

if (enableAsyncActions) {
// This is an async action.
const thenable: Thenable<mixed> = (returnValue: any);
entangleAsyncAction(transition, thenable);
}
}
Expand Down
31 changes: 31 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactAsyncActions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -1726,4 +1730,31 @@ 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 () => {
// 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']);
});
});
80 changes: 62 additions & 18 deletions packages/react/src/ReactStartTransition.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
};