Move error logging to update callback (#21737)

* Move error logging to update callback This prevents double logging for gDSFE boundaries with createRoot. * Add an explanation for the rest of duplicates
facebook · Jun 24, 2021 · 534c9c5 · 534c9c5
1 parent 51b0bec
commit 534c9c5
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 33 deletions.
diff --git a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js
@@ -166,7 +166,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
             }),
           ],
           [
-            // TODO: This is duplicated only with createRoot. Why?
+            // This is only duplicated with createRoot
+            // because it retries once with a sync render.
             expect.objectContaining({
               message: 'Boom',
             }),
@@ -181,7 +182,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
             }),
           ],
           [
-            // TODO: This is duplicated only with createRoot. Why?
+            // This is only duplicated with createRoot
+            // because it retries once with a sync render.
             expect.stringContaining('Error: Uncaught [Error: Boom]'),
             expect.objectContaining({
               message: 'Boom',
@@ -246,7 +248,8 @@ describe('ReactDOMConsoleErrorReporting', () => {
             }),
           ],
           [
-            // TODO: This is duplicated only with createRoot. Why?
+            // This is only duplicated with createRoot
+            // because it retries once with a sync render.
             expect.objectContaining({
               message: 'Boom',
             }),
@@ -261,20 +264,15 @@ describe('ReactDOMConsoleErrorReporting', () => {
             }),
           ],
           [
-            // Addendum by React:
-            expect.stringContaining(
-              'The above error occurred in the <Foo> component',
-            ),
-          ],
-          [
-            // TODO: This is duplicated only with createRoot. Why?
+            // This is only duplicated with createRoot
+            // because it retries once with a sync render.
             expect.stringContaining('Error: Uncaught [Error: Boom]'),
             expect.objectContaining({
               message: 'Boom',
             }),
           ],
           [
-            // TODO: This is duplicated only with createRoot. Why?
+            // Addendum by React:
             expect.stringContaining(
               'The above error occurred in the <Foo> component',
             ),
@@ -291,12 +289,6 @@ describe('ReactDOMConsoleErrorReporting', () => {
               message: 'Boom',
             }),
           ],
-          [
-            // TODO: This is duplicated only with createRoot. Why?
-            expect.objectContaining({
-              message: 'Boom',
-            }),
-          ],
         ]);
       }
 

diff --git a/packages/react-reconciler/src/ReactFiberThrow.new.js b/packages/react-reconciler/src/ReactFiberThrow.new.js
@@ -108,9 +108,14 @@ function createClassErrorUpdate(
   if (typeof getDerivedStateFromError === 'function') {
     const error = errorInfo.value;
     update.payload = () => {
-      logCapturedError(fiber, errorInfo);
       return getDerivedStateFromError(error);
     };
+    update.callback = () => {
+      if (__DEV__) {
+        markFailedErrorBoundaryForHotReloading(fiber);
+      }
+      logCapturedError(fiber, errorInfo);
+    };
   }
 
   const inst = fiber.stateNode;
@@ -119,16 +124,14 @@ function createClassErrorUpdate(
       if (__DEV__) {
         markFailedErrorBoundaryForHotReloading(fiber);
       }
+      logCapturedError(fiber, errorInfo);
       if (typeof getDerivedStateFromError !== 'function') {
         // To preserve the preexisting retry behavior of error boundaries,
         // we keep track of which ones already failed during this batch.
         // This gets reset before we yield back to the browser.
         // TODO: Warn in strict mode if getDerivedStateFromError is
         // not defined.
         markLegacyErrorBoundaryAsFailed(this);
-
-        // Only log here if componentDidCatch is the only error boundary method defined
-        logCapturedError(fiber, errorInfo);
       }
       const error = errorInfo.value;
       const stack = errorInfo.stack;
@@ -150,10 +153,6 @@ function createClassErrorUpdate(
         }
       }
     };
-  } else if (__DEV__) {
-    update.callback = () => {
-      markFailedErrorBoundaryForHotReloading(fiber);
-    };
   }
   return update;
 }

diff --git a/packages/react-reconciler/src/ReactFiberThrow.old.js b/packages/react-reconciler/src/ReactFiberThrow.old.js
@@ -108,9 +108,14 @@ function createClassErrorUpdate(
   if (typeof getDerivedStateFromError === 'function') {
     const error = errorInfo.value;
     update.payload = () => {
-      logCapturedError(fiber, errorInfo);
       return getDerivedStateFromError(error);
     };
+    update.callback = () => {
+      if (__DEV__) {
+        markFailedErrorBoundaryForHotReloading(fiber);
+      }
+      logCapturedError(fiber, errorInfo);
+    };
   }
 
   const inst = fiber.stateNode;
@@ -119,16 +124,14 @@ function createClassErrorUpdate(
       if (__DEV__) {
         markFailedErrorBoundaryForHotReloading(fiber);
       }
+      logCapturedError(fiber, errorInfo);
       if (typeof getDerivedStateFromError !== 'function') {
         // To preserve the preexisting retry behavior of error boundaries,
         // we keep track of which ones already failed during this batch.
         // This gets reset before we yield back to the browser.
         // TODO: Warn in strict mode if getDerivedStateFromError is
         // not defined.
         markLegacyErrorBoundaryAsFailed(this);
-
-        // Only log here if componentDidCatch is the only error boundary method defined
-        logCapturedError(fiber, errorInfo);
       }
       const error = errorInfo.value;
       const stack = errorInfo.stack;
@@ -150,10 +153,6 @@ function createClassErrorUpdate(
         }
       }
     };
-  } else if (__DEV__) {
-    update.callback = () => {
-      markFailedErrorBoundaryForHotReloading(fiber);
-    };
   }
   return update;
 }