Skip to content

Commit

Permalink
Remove invokeGuardedCallback and replay trick (facebook#28515)
Browse files Browse the repository at this point in the history
We broke the ability to "break on uncaught exceptions" by adding a
try/catch higher up in the scheduling. We're giving up on fixing that so
we can remove the replay trick inside an event handler.

The issue with that approach is that we end up double logging a lot of
errors in DEV since they get reported to the page.

It's also a lot of complexity around this feature.
  • Loading branch information
sebmarkbage authored and AndyPengc12 committed Apr 15, 2024
1 parent 3a13756 commit 9bd771a
Show file tree
Hide file tree
Showing 60 changed files with 512 additions and 1,685 deletions.
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

0 comments on commit 9bd771a

Please sign in to comment.