From 408c5f8548d28e2113b02e32853cbc5c57025683 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 3 Feb 2022 16:58:57 -0500 Subject: [PATCH] Only log hydration error if client render succeeds Similar to previous step. When an error occurs during hydration, we only want to log it if falling back to client rendering _succeeds_. If client rendering fails, the error will get reported to the nearest error boundary, so there's no need for a duplicate log. To implement this, I added a list of errors to the hydration context. If the Suspense boundary successfully completes, they are added to the main recoverable errors queue (the one I added in the previous step.) --- .../src/__tests__/ReactDOMFizzServer-test.js | 11 +++++---- ...DOMServerPartialHydration-test.internal.js | 13 +--------- .../src/ReactFiberCompleteWork.new.js | 7 ++++++ .../src/ReactFiberCompleteWork.old.js | 7 ++++++ .../src/ReactFiberHydrationContext.new.js | 24 +++++++++++++++++++ .../src/ReactFiberHydrationContext.old.js | 24 +++++++++++++++++++ .../src/ReactFiberThrow.new.js | 12 ++++------ .../src/ReactFiberThrow.old.js | 12 ++++------ .../src/ReactFiberWorkLoop.new.js | 20 +++++++++------- .../src/ReactFiberWorkLoop.old.js | 20 +++++++++------- 10 files changed, 103 insertions(+), 47 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index e16aa9cbc5649..0b40c49b66e8e 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -1983,14 +1983,15 @@ describe('ReactDOMFizzServer', () => { isClient = true; ReactDOM.hydrateRoot(container, , { onRecoverableError(error) { - // TODO: We logged a hydration error, but the same error ends up - // being thrown during the fallback to client rendering, too. Maybe - // we should only log if the client render succeeds. - Scheduler.unstable_yieldValue(error.message); + Scheduler.unstable_yieldValue( + 'Log recoverable error: ' + error.message, + ); }, }); - expect(Scheduler).toFlushAndYield(['Oops!']); + // Because we failed to recover from the error, onRecoverableError + // shouldn't be called. + expect(Scheduler).toFlushAndYield([]); expect(getVisibleChildren(container)).toEqual('Oops!'); }, ); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index 6e81765347a93..1e819091bf4c7 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -214,11 +214,7 @@ describe('ReactDOMServerPartialHydration', () => { }, }); if (gate(flags => flags.enableClientRenderFallbackOnHydrationMismatch)) { - // Hydration error is logged - expect(Scheduler).toFlushAndYield([ - 'An error occurred during hydration. The server HTML was replaced ' + - 'with client content', - ]); + Scheduler.unstable_flushAll(); } else { expect(() => { Scheduler.unstable_flushAll(); @@ -309,13 +305,6 @@ describe('ReactDOMServerPartialHydration', () => { 'Component', 'Component', 'Component', - - // Hydration mismatch errors are logged. - // TODO: This could get noisy. Is there some way to dedupe? - 'An error occurred during hydration. The server HTML was replaced with client content', - 'An error occurred during hydration. The server HTML was replaced with client content', - 'An error occurred during hydration. The server HTML was replaced with client content', - 'An error occurred during hydration. The server HTML was replaced with client content', ]); jest.runAllTimers(); diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index e8ec41d9d81f5..54f73b97e59b8 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -131,6 +131,7 @@ import { resetHydrationState, getIsHydrating, hasUnhydratedTailNodes, + queueRecoverableHydrationErrors, } from './ReactFiberHydrationContext.new'; import { enableSuspenseCallback, @@ -1099,6 +1100,12 @@ function completeWork( return null; } } + + // Successfully completed this tree. If this was a forced client render, + // there may have been recoverable errors during first hydration + // attempt. If so, add them to a queue so we can log them in the + // commit phase. + queueRecoverableHydrationErrors(workInProgress); } if ((workInProgress.flags & DidCapture) !== NoFlags) { diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js index 0a0273470a702..d6bce16bd09b4 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js @@ -131,6 +131,7 @@ import { resetHydrationState, getIsHydrating, hasUnhydratedTailNodes, + queueRecoverableHydrationErrors, } from './ReactFiberHydrationContext.old'; import { enableSuspenseCallback, @@ -1099,6 +1100,12 @@ function completeWork( return null; } } + + // Successfully completed this tree. If this was a forced client render, + // there may have been recoverable errors during first hydration + // attempt. If so, add them to a queue so we can log them in the + // commit phase. + queueRecoverableHydrationErrors(workInProgress); } if ((workInProgress.flags & DidCapture) !== NoFlags) { diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js index b6153eddccebb..1eae8d58ec363 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js @@ -77,6 +77,7 @@ import { getSuspendedTreeContext, restoreSuspendedTreeContext, } from './ReactFiberTreeContext.new'; +import {queueRecoverableErrors} from './ReactFiberWorkLoop.new'; // The deepest Fiber on the stack involved in a hydration context. // This may have been an insertion or a hydration. @@ -84,6 +85,9 @@ let hydrationParentFiber: null | Fiber = null; let nextHydratableInstance: null | HydratableInstance = null; let isHydrating: boolean = false; +// Hydration errors that were thrown inside this boundary +let hydrationErrors: Array | null = null; + function warnIfHydrating() { if (__DEV__) { if (isHydrating) { @@ -105,6 +109,7 @@ function enterHydrationState(fiber: Fiber): boolean { ); hydrationParentFiber = fiber; isHydrating = true; + hydrationErrors = null; return true; } @@ -121,6 +126,7 @@ function reenterHydrationStateFromDehydratedSuspenseInstance( ); hydrationParentFiber = fiber; isHydrating = true; + hydrationErrors = null; if (treeContext !== null) { restoreSuspendedTreeContext(fiber, treeContext); } @@ -601,10 +607,28 @@ function resetHydrationState(): void { isHydrating = false; } +export function queueRecoverableHydrationErrors(): void { + if (hydrationErrors !== null) { + // Successfully completed a forced client render. The errors that occurred + // during the hydration attempt are now recovered. We will log them in + // commit phase, once the entire tree has finished. + queueRecoverableErrors(hydrationErrors); + hydrationErrors = null; + } +} + function getIsHydrating(): boolean { return isHydrating; } +export function queueHydrationError(error: mixed): void { + if (hydrationErrors === null) { + hydrationErrors = [error]; + } else { + hydrationErrors.push(error); + } +} + export { warnIfHydrating, enterHydrationState, diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.old.js b/packages/react-reconciler/src/ReactFiberHydrationContext.old.js index 9e2518542454a..e5583916fbbd7 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.old.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.old.js @@ -77,6 +77,7 @@ import { getSuspendedTreeContext, restoreSuspendedTreeContext, } from './ReactFiberTreeContext.old'; +import {queueRecoverableErrors} from './ReactFiberWorkLoop.old'; // The deepest Fiber on the stack involved in a hydration context. // This may have been an insertion or a hydration. @@ -84,6 +85,9 @@ let hydrationParentFiber: null | Fiber = null; let nextHydratableInstance: null | HydratableInstance = null; let isHydrating: boolean = false; +// Hydration errors that were thrown inside this boundary +let hydrationErrors: Array | null = null; + function warnIfHydrating() { if (__DEV__) { if (isHydrating) { @@ -105,6 +109,7 @@ function enterHydrationState(fiber: Fiber): boolean { ); hydrationParentFiber = fiber; isHydrating = true; + hydrationErrors = null; return true; } @@ -121,6 +126,7 @@ function reenterHydrationStateFromDehydratedSuspenseInstance( ); hydrationParentFiber = fiber; isHydrating = true; + hydrationErrors = null; if (treeContext !== null) { restoreSuspendedTreeContext(fiber, treeContext); } @@ -601,10 +607,28 @@ function resetHydrationState(): void { isHydrating = false; } +export function queueRecoverableHydrationErrors(): void { + if (hydrationErrors !== null) { + // Successfully completed a forced client render. The errors that occurred + // during the hydration attempt are now recovered. We will log them in + // commit phase, once the entire tree has finished. + queueRecoverableErrors(hydrationErrors); + hydrationErrors = null; + } +} + function getIsHydrating(): boolean { return isHydrating; } +export function queueHydrationError(error: mixed): void { + if (hydrationErrors === null) { + hydrationErrors = [error]; + } else { + hydrationErrors.push(error); + } +} + export { warnIfHydrating, enterHydrationState, diff --git a/packages/react-reconciler/src/ReactFiberThrow.new.js b/packages/react-reconciler/src/ReactFiberThrow.new.js index c2b2a9a2fa5e7..6a0c70f8e6d00 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.new.js +++ b/packages/react-reconciler/src/ReactFiberThrow.new.js @@ -37,7 +37,6 @@ import { import { supportsPersistence, getOffscreenContainerProps, - logRecoverableError, } from './ReactFiberHostConfig'; import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.new'; import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode'; @@ -80,7 +79,10 @@ import { mergeLanes, pickArbitraryLane, } from './ReactFiberLane.new'; -import {getIsHydrating} from './ReactFiberHydrationContext.new'; +import { + getIsHydrating, + queueHydrationError, +} from './ReactFiberHydrationContext.new'; const PossiblyWeakMap = typeof WeakMap === 'function' ? WeakMap : Map; @@ -511,11 +513,7 @@ function throwException( // Even though the user may not be affected by this error, we should // still log it so it can be fixed. - // TODO: For now, we only log errors that occur during hydration, but we - // probably want to log any error that is recovered from without - // triggering an error boundary — or maybe even those, too. Need to - // figure out the right API. - logRecoverableError(root.errorLoggingConfig, value); + queueHydrationError(value); return; } } else { diff --git a/packages/react-reconciler/src/ReactFiberThrow.old.js b/packages/react-reconciler/src/ReactFiberThrow.old.js index 3ae5df1f93414..21ab03f4ac925 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.old.js +++ b/packages/react-reconciler/src/ReactFiberThrow.old.js @@ -37,7 +37,6 @@ import { import { supportsPersistence, getOffscreenContainerProps, - logRecoverableError, } from './ReactFiberHostConfig'; import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.old'; import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode'; @@ -80,7 +79,10 @@ import { mergeLanes, pickArbitraryLane, } from './ReactFiberLane.old'; -import {getIsHydrating} from './ReactFiberHydrationContext.old'; +import { + getIsHydrating, + queueHydrationError, +} from './ReactFiberHydrationContext.old'; const PossiblyWeakMap = typeof WeakMap === 'function' ? WeakMap : Map; @@ -511,11 +513,7 @@ function throwException( // Even though the user may not be affected by this error, we should // still log it so it can be fixed. - // TODO: For now, we only log errors that occur during hydration, but we - // probably want to log any error that is recovered from without - // triggering an error boundary — or maybe even those, too. Need to - // figure out the right API. - logRecoverableError(root.errorLoggingConfig, value); + queueHydrationError(value); return; } } else { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index edc6037a67057..f444690504316 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -909,14 +909,7 @@ function recoverFromConcurrentError(root, errorRetryLanes) { // The errors from the failed first attempt have been recovered. Add // them to the collection of recoverable errors. We'll log them in the // commit phase. - if (workInProgressRootConcurrentErrors === null) { - workInProgressRootRecoverableErrors = errorsFromFirstAttempt; - } else { - workInProgressRootConcurrentErrors = workInProgressRootConcurrentErrors.push.apply( - null, - errorsFromFirstAttempt, - ); - } + queueRecoverableErrors(errorsFromFirstAttempt); } } else { // The UI failed to recover. @@ -927,6 +920,17 @@ function recoverFromConcurrentError(root, errorRetryLanes) { return exitStatus; } +export function queueRecoverableErrors(errors: Array) { + if (workInProgressRootConcurrentErrors === null) { + workInProgressRootRecoverableErrors = errors; + } else { + workInProgressRootConcurrentErrors = workInProgressRootConcurrentErrors.push.apply( + null, + errors, + ); + } +} + function finishConcurrentRender(root, exitStatus, lanes) { switch (exitStatus) { case RootIncomplete: diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 4eb05f2ce40d0..5355d2992293a 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -909,14 +909,7 @@ function recoverFromConcurrentError(root, errorRetryLanes) { // The errors from the failed first attempt have been recovered. Add // them to the collection of recoverable errors. We'll log them in the // commit phase. - if (workInProgressRootConcurrentErrors === null) { - workInProgressRootRecoverableErrors = errorsFromFirstAttempt; - } else { - workInProgressRootConcurrentErrors = workInProgressRootConcurrentErrors.push.apply( - null, - errorsFromFirstAttempt, - ); - } + queueRecoverableErrors(errorsFromFirstAttempt); } } else { // The UI failed to recover. @@ -927,6 +920,17 @@ function recoverFromConcurrentError(root, errorRetryLanes) { return exitStatus; } +export function queueRecoverableErrors(errors: Array) { + if (workInProgressRootConcurrentErrors === null) { + workInProgressRootRecoverableErrors = errors; + } else { + workInProgressRootConcurrentErrors = workInProgressRootConcurrentErrors.push.apply( + null, + errors, + ); + } +} + function finishConcurrentRender(root, exitStatus, lanes) { switch (exitStatus) { case RootIncomplete: