From 534c9c52ec3e33ed241f7a20e264bb4571d452b3 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 24 Jun 2021 20:57:10 +0100 Subject: [PATCH] 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 --- .../ReactDOMConsoleErrorReporting-test.js | 26 +++++++------------ .../src/ReactFiberThrow.new.js | 15 +++++------ .../src/ReactFiberThrow.old.js | 15 +++++------ 3 files changed, 23 insertions(+), 33 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js index 47fbd364d3175..3507e2440981e 100644 --- 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 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 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 index e100561ed86e5..398ae98abef60 100644 --- 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,6 +124,7 @@ 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. @@ -126,9 +132,6 @@ function createClassErrorUpdate( // 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 index cdb02cb8f1886..42e2557938579 100644 --- 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,6 +124,7 @@ 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. @@ -126,9 +132,6 @@ function createClassErrorUpdate( // 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; }