From 7fec38041faf7f4691f503feccbaf5af901367e2 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 24 Jun 2021 15:48:28 +0100 Subject: [PATCH] Log and show error overlay for commit phase errors (#21723) * 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. --- .../ReactDOMConsoleErrorReporting-test.js | 24 ++++------ .../src/ReactFiberCommitWork.new.js | 47 +++++++++++++++---- .../src/ReactFiberCommitWork.old.js | 47 +++++++++++++++---- 3 files changed, 86 insertions(+), 32 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js index bbaffe5489b52..47fbd364d3175 100644 --- a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js @@ -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() { @@ -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() { @@ -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() { @@ -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() { @@ -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() { @@ -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() { @@ -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() { @@ -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() { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 60158f1f4aaa9..e7a3e157301be 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -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 | null = null; if (__DEV__) { @@ -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; @@ -186,8 +201,9 @@ function safelyCallCommitHookLayoutEffectListMount( ) { try { commitHookEffectListMount(HookLayout, current); - } catch (unmountError) { - captureCommitPhaseError(current, nearestMountedAncestor, unmountError); + } catch (error) { + reportUncaughtErrorInDEV(error); + captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -199,8 +215,9 @@ function safelyCallComponentWillUnmount( ) { try { callComponentWillUnmountWithTimer(current, instance); - } catch (unmountError) { - captureCommitPhaseError(current, nearestMountedAncestor, unmountError); + } catch (error) { + reportUncaughtErrorInDEV(error); + captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -212,8 +229,9 @@ function safelyCallComponentDidMount( ) { try { instance.componentDidMount(); - } catch (unmountError) { - captureCommitPhaseError(current, nearestMountedAncestor, unmountError); + } catch (error) { + reportUncaughtErrorInDEV(error); + captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -221,8 +239,9 @@ function safelyCallComponentDidMount( function safelyAttachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { try { commitAttachRef(current); - } catch (unmountError) { - captureCommitPhaseError(current, nearestMountedAncestor, unmountError); + } catch (error) { + reportUncaughtErrorInDEV(error); + captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -246,6 +265,7 @@ function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { ref(null); } } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } } else { @@ -262,6 +282,7 @@ function safelyCallDestroy( try { destroy(); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -323,6 +344,7 @@ function commitBeforeMutationEffects_complete() { try { commitBeforeMutationEffectsOnFiber(fiber); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2065,6 +2087,7 @@ function commitMutationEffects_begin(root: FiberRoot) { try { commitDeletion(root, childToDelete, fiber); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(childToDelete, fiber, error); } } @@ -2087,6 +2110,7 @@ function commitMutationEffects_complete(root: FiberRoot) { try { commitMutationEffectsOnFiber(fiber, root); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2329,6 +2353,7 @@ function commitLayoutMountEffects_complete( try { commitLayoutEffectOnFiber(root, current, fiber, committedLanes); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2382,6 +2407,7 @@ function commitPassiveMountEffects_complete( try { commitPassiveMountOnFiber(root, fiber); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2664,6 +2690,7 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void { try { commitHookEffectListMount(HookLayout | HookHasEffect, fiber); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -2673,6 +2700,7 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void { try { instance.componentDidMount(); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -2692,6 +2720,7 @@ function invokePassiveEffectMountInDEV(fiber: Fiber): void { try { commitHookEffectListMount(HookPassive | HookHasEffect, fiber); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -2715,6 +2744,7 @@ function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void { fiber.return, ); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -2745,6 +2775,7 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void { fiber.return, ); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index c741023f22bf5..a4ce8c644954d 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -140,6 +140,7 @@ import { } from './ReactHookEffectTags'; import {didWarnAboutReassigningProps} from './ReactFiberBeginWork.old'; import {doesFiberContain} from './ReactFiberTreeReflection'; +import {invokeGuardedCallback, clearCaughtError} from 'shared/ReactErrorUtils'; let didWarnAboutUndefinedSnapshotBeforeUpdate: Set | null = null; if (__DEV__) { @@ -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; @@ -186,8 +201,9 @@ function safelyCallCommitHookLayoutEffectListMount( ) { try { commitHookEffectListMount(HookLayout, current); - } catch (unmountError) { - captureCommitPhaseError(current, nearestMountedAncestor, unmountError); + } catch (error) { + reportUncaughtErrorInDEV(error); + captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -199,8 +215,9 @@ function safelyCallComponentWillUnmount( ) { try { callComponentWillUnmountWithTimer(current, instance); - } catch (unmountError) { - captureCommitPhaseError(current, nearestMountedAncestor, unmountError); + } catch (error) { + reportUncaughtErrorInDEV(error); + captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -212,8 +229,9 @@ function safelyCallComponentDidMount( ) { try { instance.componentDidMount(); - } catch (unmountError) { - captureCommitPhaseError(current, nearestMountedAncestor, unmountError); + } catch (error) { + reportUncaughtErrorInDEV(error); + captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -221,8 +239,9 @@ function safelyCallComponentDidMount( function safelyAttachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { try { commitAttachRef(current); - } catch (unmountError) { - captureCommitPhaseError(current, nearestMountedAncestor, unmountError); + } catch (error) { + reportUncaughtErrorInDEV(error); + captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -246,6 +265,7 @@ function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { ref(null); } } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } } else { @@ -262,6 +282,7 @@ function safelyCallDestroy( try { destroy(); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(current, nearestMountedAncestor, error); } } @@ -323,6 +344,7 @@ function commitBeforeMutationEffects_complete() { try { commitBeforeMutationEffectsOnFiber(fiber); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2065,6 +2087,7 @@ function commitMutationEffects_begin(root: FiberRoot) { try { commitDeletion(root, childToDelete, fiber); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(childToDelete, fiber, error); } } @@ -2087,6 +2110,7 @@ function commitMutationEffects_complete(root: FiberRoot) { try { commitMutationEffectsOnFiber(fiber, root); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2329,6 +2353,7 @@ function commitLayoutMountEffects_complete( try { commitLayoutEffectOnFiber(root, current, fiber, committedLanes); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2382,6 +2407,7 @@ function commitPassiveMountEffects_complete( try { commitPassiveMountOnFiber(root, fiber); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); @@ -2664,6 +2690,7 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void { try { commitHookEffectListMount(HookLayout | HookHasEffect, fiber); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -2673,6 +2700,7 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void { try { instance.componentDidMount(); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -2692,6 +2720,7 @@ function invokePassiveEffectMountInDEV(fiber: Fiber): void { try { commitHookEffectListMount(HookPassive | HookHasEffect, fiber); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -2715,6 +2744,7 @@ function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void { fiber.return, ); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } break; @@ -2745,6 +2775,7 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void { fiber.return, ); } catch (error) { + reportUncaughtErrorInDEV(error); captureCommitPhaseError(fiber, fiber.return, error); } }