Repository: facebook/react
Pull request: Log and show error overlay for commit phase errors

Fixes https://github.com/facebook/react/issues/21712.
I regressed this in https://github.com/facebook/react/pull/21666.

Before https://github.com/facebook/react/pull/2166, we'd always invokeGuardedCallback for each commit phase call. But that was too slow. Between https://github.com/facebook/react/pull/2166 and this PR, we just did try/catch. But that prevented errors from showing up in CRA error overlay or being logged to console. With this change, we only invokeGuardedCallback to rethrow already caught errors in DEV. So they show up in tooling but we don't take the perf hit for the common case.

The fix is to replicate the old behavior but only if we only catch something. This means we do it a bit later than it actually happens but assumptions about what gets logged are the same as before https://github.com/facebook/react/pull/21666.

Changed files:
filename: packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js
patch: @@ -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() {

filename: packages/react-reconciler/src/ReactFiberCommitWork.new.js
patch: @@ -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__) {
@@ -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,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);
   }
 }
 
@@ -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);
         }
       }

filename: packages/react-reconciler/src/ReactFiberCommitWork.old.js
patch: @@ -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<mixed> | 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,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);
   }
 }
 
@@ -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);
         }
       }