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

Remove invokeGuardedCallback and replay trick #28515

Merged
merged 4 commits into from
Mar 12, 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: 0 additions & 5 deletions fixtures/dom/src/toWarnDev.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,6 @@ const util = require('util');
function shouldIgnoreConsoleError(format, args) {
if (__DEV__) {
if (typeof format === 'string') {
if (format.indexOf('Error: Uncaught [') === 0) {
// This looks like an uncaught error from invokeGuardedCallback() wrapper
// in development that is reported by jsdom. Ignore because it's noisy.
return true;
}
if (format.indexOf('The above error occurred') === 0) {
// This looks like an error addendum from ReactFiberErrorLogger.
// Ignore it too.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
let ReactCache;
let createResource;
let React;
let ReactFeatureFlags;
let ReactNoop;
let Scheduler;
let Suspense;
Expand All @@ -27,9 +26,6 @@ describe('ReactCache', () => {
beforeEach(() => {
jest.resetModules();

ReactFeatureFlags = require('shared/ReactFeatureFlags');

ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
React = require('react');
Suspense = React.Suspense;
ReactCache = require('react-cache');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -870,15 +870,13 @@ describe('Timeline profiler', () => {
"--component-render-start-ErrorBoundary",
"--component-render-stop",
"--component-render-start-ExampleThatThrows",
"--component-render-start-ExampleThatThrows",
"--component-render-stop",
"--error-ExampleThatThrows-mount-Expected error",
"--render-stop",
"--render-start-32",
"--component-render-start-ErrorBoundary",
"--component-render-stop",
"--component-render-start-ExampleThatThrows",
"--component-render-start-ExampleThatThrows",
"--component-render-stop",
"--error-ExampleThatThrows-mount-Expected error",
"--render-stop",
Expand Down Expand Up @@ -2161,10 +2159,8 @@ describe('Timeline profiler', () => {
await waitForAll([
'ErrorBoundary render',
'ExampleThatThrows',
'ExampleThatThrows',
'ErrorBoundary render',
'ExampleThatThrows',
'ExampleThatThrows',
'ErrorBoundary fallback',
]);

Expand Down
26 changes: 19 additions & 7 deletions packages/react-dom-bindings/src/events/DOMPluginEventSystem.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,6 @@ import {
enableFloat,
enableFormActions,
} from 'shared/ReactFeatureFlags';
import {
invokeGuardedCallbackAndCatchFirstError,
rethrowCaughtError,
} from 'shared/ReactErrorUtils';
import {createEventListenerWrapperWithPriority} from './ReactDOMEventListener';
import {
removeEventListener,
Expand Down Expand Up @@ -234,14 +230,25 @@ export const nonDelegatedEvents: Set<DOMEventName> = new Set([
...mediaEventTypes,
]);

let hasError: boolean = false;
let caughtError: mixed = null;

function executeDispatch(
event: ReactSyntheticEvent,
listener: Function,
currentTarget: EventTarget,
): void {
const type = event.type || 'unknown-event';
event.currentTarget = currentTarget;
invokeGuardedCallbackAndCatchFirstError(type, listener, undefined, event);
try {
listener(event);
} catch (error) {
if (!hasError) {
hasError = true;
caughtError = error;
} else {
// TODO: Make sure this error gets logged somehow.
}
}
event.currentTarget = null;
}

Expand Down Expand Up @@ -283,7 +290,12 @@ export function processDispatchQueue(
// event system doesn't use pooling.
}
// This would be a good time to rethrow if any of the event handlers threw.
rethrowCaughtError();
if (hasError) {
const error = caughtError;
hasError = false;
caughtError = null;
throw error;
}
}

function dispatchEventsForPlugins(
Expand Down
22 changes: 10 additions & 12 deletions packages/react-dom/src/__tests__/InvalidEventListeners-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ describe('InvalidEventListeners', () => {
);
const node = container.firstChild;

spyOnProd(console, 'error');
console.error = jest.fn();

const uncaughtErrors = [];
function handleWindowError(e) {
Expand All @@ -70,18 +70,16 @@ describe('InvalidEventListeners', () => {
}),
);

if (!__DEV__) {
expect(console.error).toHaveBeenCalledTimes(1);
expect(console.error.mock.calls[0][0]).toEqual(
expect.objectContaining({
detail: expect.objectContaining({
message:
'Expected `onClick` listener to be a function, instead got a value of `string` type.',
}),
type: 'unhandled exception',
expect(console.error).toHaveBeenCalledTimes(1);
expect(console.error.mock.calls[0][0]).toEqual(
expect.objectContaining({
detail: expect.objectContaining({
message:
'Expected `onClick` listener to be a function, instead got a value of `string` type.',
}),
);
}
type: 'unhandled exception',
}),
);
});

it('should not prevent null listeners, at dispatch', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,21 +202,13 @@ describe('ReactBrowserEventEmitter', () => {
expect(idCallOrder[0]).toBe(CHILD);
expect(idCallOrder[1]).toBe(PARENT);
expect(idCallOrder[2]).toBe(GRANDPARENT);
expect(errorHandler).toHaveBeenCalledTimes(__DEV__ ? 2 : 1);
expect(errorHandler).toHaveBeenCalledTimes(1);
expect(errorHandler.mock.calls[0][0]).toEqual(
expect.objectContaining({
error: expect.any(Error),
message: 'Handler interrupted',
}),
);
if (__DEV__) {
expect(errorHandler.mock.calls[1][0]).toEqual(
expect.objectContaining({
error: expect.any(Error),
message: 'Handler interrupted',
}),
);
}
} finally {
window.removeEventListener('error', errorHandler);
}
Expand Down
15 changes: 0 additions & 15 deletions packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1231,13 +1231,6 @@ describe('ReactCompositeComponent', () => {
});
}).toThrow();
}).toErrorDev([
// Expect two errors because invokeGuardedCallback will dispatch an error event,
// Causing the warning to be logged again.
'Warning: No `render` method found on the RenderTextInvalidConstructor instance: ' +
'did you accidentally return an object from the constructor?',
'Warning: No `render` method found on the RenderTextInvalidConstructor instance: ' +
'did you accidentally return an object from the constructor?',
// And then two more because we retry errors.
'Warning: No `render` method found on the RenderTextInvalidConstructor instance: ' +
'did you accidentally return an object from the constructor?',
'Warning: No `render` method found on the RenderTextInvalidConstructor instance: ' +
Expand Down Expand Up @@ -1278,14 +1271,6 @@ describe('ReactCompositeComponent', () => {
});
}).toThrow();
}).toErrorDev([
// Expect two errors because invokeGuardedCallback will dispatch an error event,
// Causing the warning to be logged again.
'Warning: No `render` method found on the RenderTestUndefinedRender instance: ' +
'you may have forgotten to define `render`.',
'Warning: No `render` method found on the RenderTestUndefinedRender instance: ' +
'you may have forgotten to define `render`.',

// And then two more because we retry errors.
'Warning: No `render` method found on the RenderTestUndefinedRender instance: ' +
'you may have forgotten to define `render`.',
'Warning: No `render` method found on the RenderTestUndefinedRender instance: ' +
Expand Down
Loading