Skip to content

Commit

Permalink
Log and show error overlay for commit phase errors (#21723)
Browse files Browse the repository at this point in the history 
* Enable skipped tests from #21723

* Report uncaught errors in DEV

* Clear caught error

This is not necessary (as proven by tests) because next invokeGuardedCallback clears it anyway. But I'll keep it for consistency with other calls.
  • Loading branch information
@gaearon
gaearon authored Jun 24, 2021
1 parent 27c9c95 commit 7fec380
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 32 deletions.
24 changes: 8 additions & 16 deletions packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -313,8 +313,7 @@ describe('ReactDOMConsoleErrorReporting', () => {
}
});

// TODO: this is broken due to https://github.com/facebook/react/issues/21712.
xit('logs layout effect errors without an error boundary', () => {
it('logs layout effect errors without an error boundary', () => {
spyOnDevAndProd(console, 'error');

function Foo() {
Expand Down Expand Up @@ -382,8 +381,7 @@ describe('ReactDOMConsoleErrorReporting', () => {
}
});

// TODO: this is broken due to https://github.com/facebook/react/issues/21712.
xit('logs layout effect errors with an error boundary', () => {
it('logs layout effect errors with an error boundary', () => {
spyOnDevAndProd(console, 'error');

function Foo() {
Expand Down Expand Up @@ -453,8 +451,7 @@ describe('ReactDOMConsoleErrorReporting', () => {
}
});

// TODO: this is broken due to https://github.com/facebook/react/issues/21712.
xit('logs passive effect errors without an error boundary', () => {
it('logs passive effect errors without an error boundary', () => {
spyOnDevAndProd(console, 'error');

function Foo() {
Expand Down Expand Up @@ -522,8 +519,7 @@ describe('ReactDOMConsoleErrorReporting', () => {
}
});

// TODO: this is broken due to https://github.com/facebook/react/issues/21712.
xit('logs passive effect errors with an error boundary', () => {
it('logs passive effect errors with an error boundary', () => {
spyOnDevAndProd(console, 'error');

function Foo() {
Expand Down Expand Up @@ -827,8 +823,7 @@ describe('ReactDOMConsoleErrorReporting', () => {
}
});

// TODO: this is broken due to https://github.com/facebook/react/issues/21712.
xit('logs layout effect errors without an error boundary', () => {
it('logs layout effect errors without an error boundary', () => {
spyOnDevAndProd(console, 'error');

function Foo() {
Expand Down Expand Up @@ -898,8 +893,7 @@ describe('ReactDOMConsoleErrorReporting', () => {
}
});

// TODO: this is broken due to https://github.com/facebook/react/issues/21712.
xit('logs layout effect errors with an error boundary', () => {
it('logs layout effect errors with an error boundary', () => {
spyOnDevAndProd(console, 'error');

function Foo() {
Expand Down Expand Up @@ -972,8 +966,7 @@ describe('ReactDOMConsoleErrorReporting', () => {
}
});

// TODO: this is broken due to https://github.com/facebook/react/issues/21712.
xit('logs passive effect errors without an error boundary', () => {
it('logs passive effect errors without an error boundary', () => {
spyOnDevAndProd(console, 'error');

function Foo() {
Expand Down Expand Up @@ -1043,8 +1036,7 @@ describe('ReactDOMConsoleErrorReporting', () => {
}
});

// TODO: this is broken due to https://github.com/facebook/react/issues/21712.
xit('logs passive effect errors with an error boundary', () => {
it('logs passive effect errors with an error boundary', () => {
spyOnDevAndProd(console, 'error');

function Foo() {
Expand Down
47 changes: 39 additions & 8 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ import {
} from './ReactHookEffectTags';
import {didWarnAboutReassigningProps} from './ReactFiberBeginWork.new';
import {doesFiberContain} from './ReactFiberTreeReflection';
import {invokeGuardedCallback, clearCaughtError} from 'shared/ReactErrorUtils';

let didWarnAboutUndefinedSnapshotBeforeUpdate: Set<mixed> | null = null;
if (__DEV__) {
Expand All @@ -160,6 +161,20 @@ let nextEffect: Fiber | null = null;
let inProgressLanes: Lanes | null = null;
let inProgressRoot: FiberRoot | null = null;

function reportUncaughtErrorInDEV(error) {
// Wrapping each small part of the commit phase into a guarded
// callback is a bit too slow (https://github.com/facebook/react/pull/21666).
// But we rely on it to surface errors to DEV tools like overlays
// (https://github.com/facebook/react/issues/21712).
// As a compromise, rethrow only caught errors in a guard.
if (__DEV__) {
invokeGuardedCallback(null, () => {
throw error;
});
clearCaughtError();
}
}

const callComponentWillUnmountWithTimer = function(current, instance) {
instance.props = current.memoizedProps;
instance.state = current.memoizedState;
Expand All @@ -186,8 +201,9 @@ function safelyCallCommitHookLayoutEffectListMount(
) {
try {
commitHookEffectListMount(HookLayout, current);
} catch (unmountError) {
captureCommitPhaseError(current, nearestMountedAncestor, unmountError);
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(current, nearestMountedAncestor, error);
}
}

Expand All @@ -199,8 +215,9 @@ function safelyCallComponentWillUnmount(
) {
try {
callComponentWillUnmountWithTimer(current, instance);
} catch (unmountError) {
captureCommitPhaseError(current, nearestMountedAncestor, unmountError);
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(current, nearestMountedAncestor, error);
}
}

Expand All @@ -212,17 +229,19 @@ function safelyCallComponentDidMount(
) {
try {
instance.componentDidMount();
} catch (unmountError) {
captureCommitPhaseError(current, nearestMountedAncestor, unmountError);
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(current, nearestMountedAncestor, error);
}
}

// Capture errors so they don't interrupt mounting.
function safelyAttachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
try {
commitAttachRef(current);
} catch (unmountError) {
captureCommitPhaseError(current, nearestMountedAncestor, unmountError);
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(current, nearestMountedAncestor, error);
}
}

Expand All @@ -246,6 +265,7 @@ function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
ref(null);
}
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(current, nearestMountedAncestor, error);
}
} else {
Expand All @@ -262,6 +282,7 @@ function safelyCallDestroy(
try {
destroy();
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(current, nearestMountedAncestor, error);
}
}
Expand Down Expand Up @@ -323,6 +344,7 @@ function commitBeforeMutationEffects_complete() {
try {
commitBeforeMutationEffectsOnFiber(fiber);
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(fiber, fiber.return, error);
}
resetCurrentDebugFiberInDEV();
Expand Down Expand Up @@ -2065,6 +2087,7 @@ function commitMutationEffects_begin(root: FiberRoot) {
try {
commitDeletion(root, childToDelete, fiber);
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(childToDelete, fiber, error);
}
}
Expand All @@ -2087,6 +2110,7 @@ function commitMutationEffects_complete(root: FiberRoot) {
try {
commitMutationEffectsOnFiber(fiber, root);
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(fiber, fiber.return, error);
}
resetCurrentDebugFiberInDEV();
Expand Down Expand Up @@ -2329,6 +2353,7 @@ function commitLayoutMountEffects_complete(
try {
commitLayoutEffectOnFiber(root, current, fiber, committedLanes);
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(fiber, fiber.return, error);
}
resetCurrentDebugFiberInDEV();
Expand Down Expand Up @@ -2382,6 +2407,7 @@ function commitPassiveMountEffects_complete(
try {
commitPassiveMountOnFiber(root, fiber);
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(fiber, fiber.return, error);
}
resetCurrentDebugFiberInDEV();
Expand Down Expand Up @@ -2664,6 +2690,7 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void {
try {
commitHookEffectListMount(HookLayout | HookHasEffect, fiber);
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(fiber, fiber.return, error);
}
break;
Expand All @@ -2673,6 +2700,7 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void {
try {
instance.componentDidMount();
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(fiber, fiber.return, error);
}
break;
Expand All @@ -2692,6 +2720,7 @@ function invokePassiveEffectMountInDEV(fiber: Fiber): void {
try {
commitHookEffectListMount(HookPassive | HookHasEffect, fiber);
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(fiber, fiber.return, error);
}
break;
Expand All @@ -2715,6 +2744,7 @@ function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void {
fiber.return,
);
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(fiber, fiber.return, error);
}
break;
Expand Down Expand Up @@ -2745,6 +2775,7 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void {
fiber.return,
);
} catch (error) {
reportUncaughtErrorInDEV(error);
captureCommitPhaseError(fiber, fiber.return, error);
}
}
Expand Down
Loading

0 comments on commit 7fec380

Please sign in to comment.