From 33cfef82430f2f9db218e1c7ea440f21aafc423a Mon Sep 17 00:00:00 2001 From: Marcel Laverdet Date: Sat, 19 Feb 2022 18:59:31 -0600 Subject: [PATCH 1/2] Failing test for react#23331 --- .../src/__tests__/ReactDOMFizzServer-test.js | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 01397d00004fc..de17e49f156d5 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -301,6 +301,84 @@ describe('ReactDOMFizzServer', () => { ); }); + // @gate experimental + it('should asynchronously load a lazy component with sibling after', async () => { + const makeApp = () => { + let resolve; + const imports = new Promise(r => { + resolve = () => r({default: () => async}); + }); + const Lazy = React.lazy(() => imports); + + // Test passes if you change: + // after + // to: + // after + const App = () => ( +
+ Loading...}> + + after + +
+ ); + + return [App, resolve]; + }; + + // Server-side + const [App, resolve] = makeApp(); + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + pipe(writable); + }); + expect(getVisibleChildren(container)).toEqual( +
+ Loading... +
, + ); + await act(async () => { + resolve(); + }); + expect(getVisibleChildren(container)).toEqual( +
+ async + after +
, + ); + + // Client-side + const [HydrateApp, hydrateResolve] = makeApp(); + await act(async () => { + ReactDOM.hydrateRoot(container, ); + // Throws after flushAll: + // Warning: Prop `id` did not match. Server: "async" Client: "after" + // at span + // at Suspense + // at div + // at App + Scheduler.unstable_flushAll(); + }); + + // nb: Honestly not really sure whether this should expect "loading..." or "async" + expect(getVisibleChildren(container)).toEqual( +
+ async + after +
, + ); + + await act(async () => { + hydrateResolve(); + }); + expect(getVisibleChildren(container)).toEqual( +
+ async + after +
, + ); + }); + // @gate experimental it('should support nonce scripts', async () => { CSPnonce = 'R4nd0m'; From be112b2facf455e7eca50c26c1db4aee2996dcc7 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 25 Feb 2022 17:13:11 -0500 Subject: [PATCH 2/2] Don't warn on hydration mismatch if suspended When something suspends during hydration, we continue rendering the siblings to warm up the cache and fire off any lazy network requests. However, if there are any mismatches while rendering the siblings, it's likely a false positive caused by the earlier suspended component. So we should suppress any hydration warnings until the tree no longer suspends. Fixes #23332 --- .../src/__tests__/ReactDOMFizzServer-test.js | 14 +-------- .../react-dom/src/client/ReactDOMComponent.js | 31 ++++++++++++------- .../src/client/ReactDOMHostConfig.js | 19 ++++++++++-- .../src/ReactFiberHydrationContext.new.js | 27 +++++++++++++++- .../src/ReactFiberHydrationContext.old.js | 27 +++++++++++++++- .../src/ReactFiberThrow.new.js | 3 ++ .../src/ReactFiberThrow.old.js | 3 ++ 7 files changed, 96 insertions(+), 28 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index de17e49f156d5..0b14e567c7e0d 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -302,7 +302,7 @@ describe('ReactDOMFizzServer', () => { }); // @gate experimental - it('should asynchronously load a lazy component with sibling after', async () => { + it('#23331: does not warn about hydration mismatches if something suspended in an earlier sibling', async () => { const makeApp = () => { let resolve; const imports = new Promise(r => { @@ -310,10 +310,6 @@ describe('ReactDOMFizzServer', () => { }); const Lazy = React.lazy(() => imports); - // Test passes if you change: - // after - // to: - // after const App = () => (
Loading...}> @@ -351,16 +347,8 @@ describe('ReactDOMFizzServer', () => { const [HydrateApp, hydrateResolve] = makeApp(); await act(async () => { ReactDOM.hydrateRoot(container, ); - // Throws after flushAll: - // Warning: Prop `id` did not match. Server: "async" Client: "after" - // at span - // at Suspense - // at div - // at App - Scheduler.unstable_flushAll(); }); - // nb: Honestly not really sure whether this should expect "loading..." or "async" expect(getVisibleChildren(container)).toEqual(
async diff --git a/packages/react-dom/src/client/ReactDOMComponent.js b/packages/react-dom/src/client/ReactDOMComponent.js index 767b200f29093..1552629da1744 100644 --- a/packages/react-dom/src/client/ReactDOMComponent.js +++ b/packages/react-dom/src/client/ReactDOMComponent.js @@ -230,6 +230,7 @@ export function checkForUnmatchedText( serverText: string, clientText: string | number, isConcurrentMode: boolean, + shouldWarnDev: boolean, ) { const normalizedClientText = normalizeMarkupForTextOrAttribute(clientText); const normalizedServerText = normalizeMarkupForTextOrAttribute(serverText); @@ -237,14 +238,16 @@ export function checkForUnmatchedText( return; } - if (__DEV__) { - if (!didWarnInvalidHydration) { - didWarnInvalidHydration = true; - console.error( - 'Text content did not match. Server: "%s" Client: "%s"', - normalizedServerText, - normalizedClientText, - ); + if (shouldWarnDev) { + if (__DEV__) { + if (!didWarnInvalidHydration) { + didWarnInvalidHydration = true; + console.error( + 'Text content did not match. Server: "%s" Client: "%s"', + normalizedServerText, + normalizedClientText, + ); + } } } @@ -866,6 +869,7 @@ export function diffHydratedProperties( parentNamespace: string, rootContainerElement: Element | Document, isConcurrentMode: boolean, + shouldWarnDev: boolean, ): null | Array { let isCustomComponentTag; let extraAttributeNames: Set; @@ -985,6 +989,7 @@ export function diffHydratedProperties( domElement.textContent, nextProp, isConcurrentMode, + shouldWarnDev, ); } updatePayload = [CHILDREN, nextProp]; @@ -996,6 +1001,7 @@ export function diffHydratedProperties( domElement.textContent, nextProp, isConcurrentMode, + shouldWarnDev, ); } updatePayload = [CHILDREN, '' + nextProp]; @@ -1011,6 +1017,7 @@ export function diffHydratedProperties( } } } else if ( + shouldWarnDev && __DEV__ && // Convince Flow we've calculated it (it's DEV-only in this method.) typeof isCustomComponentTag === 'boolean' @@ -1142,10 +1149,12 @@ export function diffHydratedProperties( } if (__DEV__) { - // $FlowFixMe - Should be inferred as not undefined. - if (extraAttributeNames.size > 0 && !suppressHydrationWarning) { + if (shouldWarnDev) { // $FlowFixMe - Should be inferred as not undefined. - warnForExtraAttributes(extraAttributeNames); + if (extraAttributeNames.size > 0 && !suppressHydrationWarning) { + // $FlowFixMe - Should be inferred as not undefined. + warnForExtraAttributes(extraAttributeNames); + } } } diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 4863a16eda68e..97eb494ecba2a 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -786,6 +786,7 @@ export function hydrateInstance( rootContainerInstance: Container, hostContext: HostContext, internalInstanceHandle: Object, + shouldWarnDev: boolean, ): null | Array { precacheFiberNode(internalInstanceHandle, instance); // TODO: Possibly defer this until the commit phase where all the events @@ -811,6 +812,7 @@ export function hydrateInstance( parentNamespace, rootContainerInstance, isConcurrentMode, + shouldWarnDev, ); } @@ -818,6 +820,7 @@ export function hydrateTextInstance( textInstance: TextInstance, text: string, internalInstanceHandle: Object, + shouldWarnDev: boolean, ): boolean { precacheFiberNode(internalInstanceHandle, textInstance); @@ -924,7 +927,13 @@ export function didNotMatchHydratedContainerTextInstance( text: string, isConcurrentMode: boolean, ) { - checkForUnmatchedText(textInstance.nodeValue, text, isConcurrentMode); + const shouldWarnDev = true; + checkForUnmatchedText( + textInstance.nodeValue, + text, + isConcurrentMode, + shouldWarnDev, + ); } export function didNotMatchHydratedTextInstance( @@ -936,7 +945,13 @@ export function didNotMatchHydratedTextInstance( isConcurrentMode: boolean, ) { if (parentProps[SUPPRESS_HYDRATION_WARNING] !== true) { - checkForUnmatchedText(textInstance.nodeValue, text, isConcurrentMode); + const shouldWarnDev = true; + checkForUnmatchedText( + textInstance.nodeValue, + text, + isConcurrentMode, + shouldWarnDev, + ); } } diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js index 6905f58e2ab1b..6a7a175d6cff3 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js @@ -84,6 +84,7 @@ import {queueRecoverableErrors} from './ReactFiberWorkLoop.new'; let hydrationParentFiber: null | Fiber = null; let nextHydratableInstance: null | HydratableInstance = null; let isHydrating: boolean = false; +let didSuspend: boolean = false; // Hydration errors that were thrown inside this boundary let hydrationErrors: Array | null = null; @@ -98,6 +99,12 @@ function warnIfHydrating() { } } +export function markDidSuspendWhileHydratingDEV() { + if (__DEV__) { + didSuspend = true; + } +} + function enterHydrationState(fiber: Fiber): boolean { if (!supportsHydration) { return false; @@ -110,6 +117,7 @@ function enterHydrationState(fiber: Fiber): boolean { hydrationParentFiber = fiber; isHydrating = true; hydrationErrors = null; + didSuspend = false; return true; } @@ -127,6 +135,7 @@ function reenterHydrationStateFromDehydratedSuspenseInstance( hydrationParentFiber = fiber; isHydrating = true; hydrationErrors = null; + didSuspend = false; if (treeContext !== null) { restoreSuspendedTreeContext(fiber, treeContext); } @@ -185,6 +194,13 @@ function deleteHydratableInstance( function warnNonhydratedInstance(returnFiber: Fiber, fiber: Fiber) { if (__DEV__) { + if (didSuspend) { + // Inside a boundary that already suspended. We're currently rendering the + // siblings of a suspended node. The mismatch may be due to the missing + // data, so it's probably a false positive. + return; + } + switch (returnFiber.tag) { case HostRoot: { const parentContainer = returnFiber.stateNode.containerInfo; @@ -418,6 +434,7 @@ function prepareToHydrateHostInstance( } const instance: Instance = fiber.stateNode; + const shouldWarnIfMismatchDev = !didSuspend; const updatePayload = hydrateInstance( instance, fiber.type, @@ -425,6 +442,7 @@ function prepareToHydrateHostInstance( rootContainerInstance, hostContext, fiber, + shouldWarnIfMismatchDev, ); // TODO: Type this specific to this type of component. fiber.updateQueue = (updatePayload: any); @@ -446,7 +464,13 @@ function prepareToHydrateHostTextInstance(fiber: Fiber): boolean { const textInstance: TextInstance = fiber.stateNode; const textContent: string = fiber.memoizedProps; - const shouldUpdate = hydrateTextInstance(textInstance, textContent, fiber); + const shouldWarnIfMismatchDev = !didSuspend; + const shouldUpdate = hydrateTextInstance( + textInstance, + textContent, + fiber, + shouldWarnIfMismatchDev, + ); if (shouldUpdate) { // We assume that prepareToHydrateHostTextInstance is called in a context where the // hydration parent is the parent host component of this host text. @@ -616,6 +640,7 @@ function resetHydrationState(): void { hydrationParentFiber = null; nextHydratableInstance = null; isHydrating = false; + didSuspend = false; } export function upgradeHydrationErrorsToRecoverable(): void { diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.old.js b/packages/react-reconciler/src/ReactFiberHydrationContext.old.js index b9e88d3a21af0..8c334924943ab 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.old.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.old.js @@ -84,6 +84,7 @@ import {queueRecoverableErrors} from './ReactFiberWorkLoop.old'; let hydrationParentFiber: null | Fiber = null; let nextHydratableInstance: null | HydratableInstance = null; let isHydrating: boolean = false; +let didSuspend: boolean = false; // Hydration errors that were thrown inside this boundary let hydrationErrors: Array | null = null; @@ -98,6 +99,12 @@ function warnIfHydrating() { } } +export function markDidSuspendWhileHydratingDEV() { + if (__DEV__) { + didSuspend = true; + } +} + function enterHydrationState(fiber: Fiber): boolean { if (!supportsHydration) { return false; @@ -110,6 +117,7 @@ function enterHydrationState(fiber: Fiber): boolean { hydrationParentFiber = fiber; isHydrating = true; hydrationErrors = null; + didSuspend = false; return true; } @@ -127,6 +135,7 @@ function reenterHydrationStateFromDehydratedSuspenseInstance( hydrationParentFiber = fiber; isHydrating = true; hydrationErrors = null; + didSuspend = false; if (treeContext !== null) { restoreSuspendedTreeContext(fiber, treeContext); } @@ -185,6 +194,13 @@ function deleteHydratableInstance( function warnNonhydratedInstance(returnFiber: Fiber, fiber: Fiber) { if (__DEV__) { + if (didSuspend) { + // Inside a boundary that already suspended. We're currently rendering the + // siblings of a suspended node. The mismatch may be due to the missing + // data, so it's probably a false positive. + return; + } + switch (returnFiber.tag) { case HostRoot: { const parentContainer = returnFiber.stateNode.containerInfo; @@ -418,6 +434,7 @@ function prepareToHydrateHostInstance( } const instance: Instance = fiber.stateNode; + const shouldWarnIfMismatchDev = !didSuspend; const updatePayload = hydrateInstance( instance, fiber.type, @@ -425,6 +442,7 @@ function prepareToHydrateHostInstance( rootContainerInstance, hostContext, fiber, + shouldWarnIfMismatchDev, ); // TODO: Type this specific to this type of component. fiber.updateQueue = (updatePayload: any); @@ -446,7 +464,13 @@ function prepareToHydrateHostTextInstance(fiber: Fiber): boolean { const textInstance: TextInstance = fiber.stateNode; const textContent: string = fiber.memoizedProps; - const shouldUpdate = hydrateTextInstance(textInstance, textContent, fiber); + const shouldWarnIfMismatchDev = !didSuspend; + const shouldUpdate = hydrateTextInstance( + textInstance, + textContent, + fiber, + shouldWarnIfMismatchDev, + ); if (shouldUpdate) { // We assume that prepareToHydrateHostTextInstance is called in a context where the // hydration parent is the parent host component of this host text. @@ -616,6 +640,7 @@ function resetHydrationState(): void { hydrationParentFiber = null; nextHydratableInstance = null; isHydrating = false; + didSuspend = false; } export function upgradeHydrationErrorsToRecoverable(): void { diff --git a/packages/react-reconciler/src/ReactFiberThrow.new.js b/packages/react-reconciler/src/ReactFiberThrow.new.js index cc5e3215c786d..815cb25ef045f 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.new.js +++ b/packages/react-reconciler/src/ReactFiberThrow.new.js @@ -83,6 +83,7 @@ import { } from './ReactFiberLane.new'; import { getIsHydrating, + markDidSuspendWhileHydratingDEV, queueHydrationError, } from './ReactFiberHydrationContext.new'; @@ -513,6 +514,8 @@ function throwException( } else { // This is a regular error, not a Suspense wakeable. if (getIsHydrating() && sourceFiber.mode & ConcurrentMode) { + markDidSuspendWhileHydratingDEV(); + const suspenseBoundary = getNearestSuspenseBoundaryToCapture(returnFiber); // If the error was thrown during hydration, we may be able to recover by // discarding the dehydrated content and switching to a client render. diff --git a/packages/react-reconciler/src/ReactFiberThrow.old.js b/packages/react-reconciler/src/ReactFiberThrow.old.js index 727c613d7622f..ec89f5ab0cd5e 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.old.js +++ b/packages/react-reconciler/src/ReactFiberThrow.old.js @@ -83,6 +83,7 @@ import { } from './ReactFiberLane.old'; import { getIsHydrating, + markDidSuspendWhileHydratingDEV, queueHydrationError, } from './ReactFiberHydrationContext.old'; @@ -513,6 +514,8 @@ function throwException( } else { // This is a regular error, not a Suspense wakeable. if (getIsHydrating() && sourceFiber.mode & ConcurrentMode) { + markDidSuspendWhileHydratingDEV(); + const suspenseBoundary = getNearestSuspenseBoundaryToCapture(returnFiber); // If the error was thrown during hydration, we may be able to recover by // discarding the dehydrated content and switching to a client render.