From 8fa41ffa275cae4895b650b0c3b5e8acdbb5055d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Thu, 16 Mar 2023 22:39:17 -0400 Subject: [PATCH] Don't "fix up" mismatched text content with suppressedHydrationWarning (#26391) In concurrent mode we error if child nodes mismatches which triggers a recreation of the whole hydration boundary. This ensures that we don't replay the wrong thing, transform state or other security issues. For text content, we respect `suppressedHydrationWarning` to allow for things like `
{timestamp}
` to ignore the timestamp. This mode actually still patches up the text content to be the client rendered content. In principle we shouldn't have to do that because either value should be ok, and arguably it's better not to trigger layout thrash after the fact. We do have a lot of code still to deal with patching up the tree because that's what legacy mode does which is still in the code base. When we delete legacy mode we would still be stuck with a lot of it just to deal with this case. Therefore I propose that we change the semantics to not patch up hydration errors for text nodes. We already don't for attributes. --- .../src/client/ReactDOMComponent.js | 353 +++++++++--------- .../src/__tests__/ReactDOMFizzServer-test.js | 2 +- ...actDOMFizzSuppressHydrationWarning-test.js | 36 +- .../src/__tests__/ReactDOMFloat-test.js | 2 +- .../src/ReactFiberHydrationContext.js | 10 + 5 files changed, 205 insertions(+), 198 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js index 57689ca6145c7..fd6dc39b2a84d 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -893,11 +893,9 @@ export function diffHydratedProperties( shouldWarnDev: boolean, parentNamespaceDev: string, ): null | Array { - let isCustomComponentTag; let extraAttributeNames: Set; if (__DEV__) { - isCustomComponentTag = isCustomComponent(tag, rawProps); validatePropertiesInDevelopment(tag, rawProps); } @@ -963,6 +961,10 @@ export function diffHydratedProperties( break; } + if (rawProps.hasOwnProperty('onScroll')) { + listenToNonDelegatedEvent('scroll', domElement); + } + assertValidProps(tag, rawProps); if (__DEV__) { @@ -988,207 +990,196 @@ export function diffHydratedProperties( } let updatePayload = null; - for (const propKey in rawProps) { - if (!rawProps.hasOwnProperty(propKey)) { - continue; - } - const nextProp = rawProps[propKey]; - if (propKey === CHILDREN) { - // For text content children we compare against textContent. This - // might match additional HTML that is hidden when we read it using - // textContent. E.g. "foo" will match "foo" but that still - // satisfies our requirement. Our requirement is not to produce perfect - // HTML and attributes. Ideally we should preserve structure but it's - // ok not to if the visible content is still enough to indicate what - // even listeners these nodes might be wired up to. - // TODO: Warn if there is more than a single textNode as a child. - // TODO: Should we use domElement.firstChild.nodeValue to compare? - if (typeof nextProp === 'string') { - if (domElement.textContent !== nextProp) { - if (rawProps[SUPPRESS_HYDRATION_WARNING] !== true) { - checkForUnmatchedText( - domElement.textContent, - nextProp, - isConcurrentMode, - shouldWarnDev, - ); - } - updatePayload = [CHILDREN, nextProp]; - } - } else if (typeof nextProp === 'number') { - if (domElement.textContent !== '' + nextProp) { - if (rawProps[SUPPRESS_HYDRATION_WARNING] !== true) { - checkForUnmatchedText( - domElement.textContent, - nextProp, - isConcurrentMode, - shouldWarnDev, - ); - } - updatePayload = [CHILDREN, '' + nextProp]; - } + + const children = rawProps.children; + // For text content children we compare against textContent. This + // might match additional HTML that is hidden when we read it using + // textContent. E.g. "foo" will match "foo" but that still + // satisfies our requirement. Our requirement is not to produce perfect + // HTML and attributes. Ideally we should preserve structure but it's + // ok not to if the visible content is still enough to indicate what + // even listeners these nodes might be wired up to. + // TODO: Warn if there is more than a single textNode as a child. + // TODO: Should we use domElement.firstChild.nodeValue to compare? + if (typeof children === 'string' || typeof children === 'number') { + if (domElement.textContent !== '' + children) { + if (rawProps[SUPPRESS_HYDRATION_WARNING] !== true) { + checkForUnmatchedText( + domElement.textContent, + children, + isConcurrentMode, + shouldWarnDev, + ); } - } else if (registrationNameDependencies.hasOwnProperty(propKey)) { - if (nextProp != null) { - if (__DEV__ && typeof nextProp !== 'function') { - warnForInvalidEventListener(propKey, nextProp); - } - if (propKey === 'onScroll') { - listenToNonDelegatedEvent('scroll', domElement); - } + if (!isConcurrentMode) { + updatePayload = [CHILDREN, children]; } - } else if ( - shouldWarnDev && - __DEV__ && - // Convince Flow we've calculated it (it's DEV-only in this method.) - typeof isCustomComponentTag === 'boolean' - ) { - // Validate that the properties correspond to their expected values. - let serverValue; - const propertyInfo = - isCustomComponentTag && enableCustomElementPropertySupport - ? null - : getPropertyInfo(propKey); - if (rawProps[SUPPRESS_HYDRATION_WARNING] === true) { - // Don't bother comparing. We're ignoring all these warnings. - } else if ( - propKey === SUPPRESS_CONTENT_EDITABLE_WARNING || - propKey === SUPPRESS_HYDRATION_WARNING || - // Controlled attributes are not validated - // TODO: Only ignore them on controlled tags. - propKey === 'value' || - propKey === 'checked' || - propKey === 'selected' - ) { - // Noop - } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { - const serverHTML = domElement.innerHTML; - const nextHtml = nextProp ? nextProp[HTML] : undefined; - if (nextHtml != null) { - const expectedHTML = normalizeHTML(domElement, nextHtml); - if (expectedHTML !== serverHTML) { - warnForPropDifference(propKey, serverHTML, expectedHTML); + } + } + + if (__DEV__ && shouldWarnDev) { + const isCustomComponentTag = isCustomComponent(tag, rawProps); + + for (const propKey in rawProps) { + if (!rawProps.hasOwnProperty(propKey)) { + continue; + } + const nextProp = rawProps[propKey]; + if (propKey === CHILDREN) { + // Checked above already + } else if (registrationNameDependencies.hasOwnProperty(propKey)) { + if (nextProp != null) { + if (typeof nextProp !== 'function') { + warnForInvalidEventListener(propKey, nextProp); } } - } else if (propKey === STYLE) { - // $FlowFixMe - Should be inferred as not undefined. - extraAttributeNames.delete(propKey); - - if (canDiffStyleForHydrationWarning) { - const expectedStyle = createDangerousStringForStyles(nextProp); - serverValue = domElement.getAttribute('style'); - if (expectedStyle !== serverValue) { - warnForPropDifference(propKey, serverValue, expectedStyle); + } else { + // Validate that the properties correspond to their expected values. + let serverValue; + const propertyInfo = + isCustomComponentTag && enableCustomElementPropertySupport + ? null + : getPropertyInfo(propKey); + if (rawProps[SUPPRESS_HYDRATION_WARNING] === true) { + // Don't bother comparing. We're ignoring all these warnings. + } else if ( + propKey === SUPPRESS_CONTENT_EDITABLE_WARNING || + propKey === SUPPRESS_HYDRATION_WARNING || + // Controlled attributes are not validated + // TODO: Only ignore them on controlled tags. + propKey === 'value' || + propKey === 'checked' || + propKey === 'selected' + ) { + // Noop + } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { + const serverHTML = domElement.innerHTML; + const nextHtml = nextProp ? nextProp[HTML] : undefined; + if (nextHtml != null) { + const expectedHTML = normalizeHTML(domElement, nextHtml); + if (expectedHTML !== serverHTML) { + warnForPropDifference(propKey, serverHTML, expectedHTML); + } } - } - } else if ( - enableCustomElementPropertySupport && - isCustomComponentTag && - (propKey === 'offsetParent' || - propKey === 'offsetTop' || - propKey === 'offsetLeft' || - propKey === 'offsetWidth' || - propKey === 'offsetHeight' || - propKey === 'isContentEditable' || - propKey === 'outerText' || - propKey === 'outerHTML') - ) { - // $FlowFixMe - Should be inferred as not undefined. - extraAttributeNames.delete(propKey.toLowerCase()); - if (__DEV__) { - console.error( - 'Assignment to read-only property will result in a no-op: `%s`', - propKey, - ); - } - } else if (isCustomComponentTag && !enableCustomElementPropertySupport) { - // $FlowFixMe - Should be inferred as not undefined. - extraAttributeNames.delete(propKey.toLowerCase()); - serverValue = getValueForAttribute( - domElement, - propKey, - nextProp, - isCustomComponentTag, - ); + } else if (propKey === STYLE) { + // $FlowFixMe - Should be inferred as not undefined. + extraAttributeNames.delete(propKey); - if (nextProp !== serverValue) { - warnForPropDifference(propKey, serverValue, nextProp); - } - } else if ( - !shouldIgnoreAttribute(propKey, propertyInfo, isCustomComponentTag) && - !shouldRemoveAttribute( - propKey, - nextProp, - propertyInfo, - isCustomComponentTag, - ) - ) { - let isMismatchDueToBadCasing = false; - if (propertyInfo !== null) { + if (canDiffStyleForHydrationWarning) { + const expectedStyle = createDangerousStringForStyles(nextProp); + serverValue = domElement.getAttribute('style'); + if (expectedStyle !== serverValue) { + warnForPropDifference(propKey, serverValue, expectedStyle); + } + } + } else if ( + enableCustomElementPropertySupport && + isCustomComponentTag && + (propKey === 'offsetParent' || + propKey === 'offsetTop' || + propKey === 'offsetLeft' || + propKey === 'offsetWidth' || + propKey === 'offsetHeight' || + propKey === 'isContentEditable' || + propKey === 'outerText' || + propKey === 'outerHTML') + ) { + // $FlowFixMe - Should be inferred as not undefined. + extraAttributeNames.delete(propKey.toLowerCase()); + if (__DEV__) { + console.error( + 'Assignment to read-only property will result in a no-op: `%s`', + propKey, + ); + } + } else if ( + isCustomComponentTag && + !enableCustomElementPropertySupport + ) { // $FlowFixMe - Should be inferred as not undefined. - extraAttributeNames.delete(propertyInfo.attributeName); - serverValue = getValueForProperty( + extraAttributeNames.delete(propKey.toLowerCase()); + serverValue = getValueForAttribute( domElement, propKey, nextProp, - propertyInfo, + isCustomComponentTag, ); - } else { - let ownNamespaceDev = parentNamespaceDev; - if (ownNamespaceDev === HTML_NAMESPACE) { - ownNamespaceDev = getIntrinsicNamespace(tag); + + if (nextProp !== serverValue) { + warnForPropDifference(propKey, serverValue, nextProp); } - if (ownNamespaceDev === HTML_NAMESPACE) { + } else if ( + !shouldIgnoreAttribute(propKey, propertyInfo, isCustomComponentTag) && + !shouldRemoveAttribute( + propKey, + nextProp, + propertyInfo, + isCustomComponentTag, + ) + ) { + let isMismatchDueToBadCasing = false; + if (propertyInfo !== null) { // $FlowFixMe - Should be inferred as not undefined. - extraAttributeNames.delete(propKey.toLowerCase()); + extraAttributeNames.delete(propertyInfo.attributeName); + serverValue = getValueForProperty( + domElement, + propKey, + nextProp, + propertyInfo, + ); } else { - const standardName = getPossibleStandardName(propKey); - if (standardName !== null && standardName !== propKey) { - // If an SVG prop is supplied with bad casing, it will - // be successfully parsed from HTML, but will produce a mismatch - // (and would be incorrectly rendered on the client). - // However, we already warn about bad casing elsewhere. - // So we'll skip the misleading extra mismatch warning in this case. - isMismatchDueToBadCasing = true; + let ownNamespaceDev = parentNamespaceDev; + if (ownNamespaceDev === HTML_NAMESPACE) { + ownNamespaceDev = getIntrinsicNamespace(tag); + } + if (ownNamespaceDev === HTML_NAMESPACE) { + // $FlowFixMe - Should be inferred as not undefined. + extraAttributeNames.delete(propKey.toLowerCase()); + } else { + const standardName = getPossibleStandardName(propKey); + if (standardName !== null && standardName !== propKey) { + // If an SVG prop is supplied with bad casing, it will + // be successfully parsed from HTML, but will produce a mismatch + // (and would be incorrectly rendered on the client). + // However, we already warn about bad casing elsewhere. + // So we'll skip the misleading extra mismatch warning in this case. + isMismatchDueToBadCasing = true; + // $FlowFixMe - Should be inferred as not undefined. + extraAttributeNames.delete(standardName); + } // $FlowFixMe - Should be inferred as not undefined. - extraAttributeNames.delete(standardName); + extraAttributeNames.delete(propKey); } - // $FlowFixMe - Should be inferred as not undefined. - extraAttributeNames.delete(propKey); + serverValue = getValueForAttribute( + domElement, + propKey, + nextProp, + isCustomComponentTag, + ); } - serverValue = getValueForAttribute( - domElement, - propKey, - nextProp, - isCustomComponentTag, - ); - } - const dontWarnCustomElement = - enableCustomElementPropertySupport && - isCustomComponentTag && - (typeof nextProp === 'function' || typeof nextProp === 'object'); - if ( - !dontWarnCustomElement && - nextProp !== serverValue && - !isMismatchDueToBadCasing - ) { - warnForPropDifference(propKey, serverValue, nextProp); + const dontWarnCustomElement = + enableCustomElementPropertySupport && + isCustomComponentTag && + (typeof nextProp === 'function' || typeof nextProp === 'object'); + if ( + !dontWarnCustomElement && + nextProp !== serverValue && + !isMismatchDueToBadCasing + ) { + warnForPropDifference(propKey, serverValue, nextProp); + } } } } - } - if (__DEV__) { - if (shouldWarnDev) { - if ( - // $FlowFixMe - Should be inferred as not undefined. - extraAttributeNames.size > 0 && - rawProps[SUPPRESS_HYDRATION_WARNING] !== true - ) { - // $FlowFixMe - Should be inferred as not undefined. - warnForExtraAttributes(extraAttributeNames); - } + if ( + // $FlowFixMe - Should be inferred as not undefined. + extraAttributeNames.size > 0 && + rawProps[SUPPRESS_HYDRATION_WARNING] !== true + ) { + // $FlowFixMe - Should be inferred as not undefined. + warnForExtraAttributes(extraAttributeNames); } } diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index ecb437f72df1f..55c629d77d4a9 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -3813,7 +3813,7 @@ describe('ReactDOMFizzServer', () => { 'Logged recoverable error: There was an error while hydrating this Suspense boundary. Switched to client rendering.', ]); }).toErrorDev( - 'Warning: Prop `name` did not match. Server: "initial" Client: "replaced"', + 'Warning: Text content did not match. Server: "initial" Client: "replaced', ); expect(getVisibleChildren(container)).toEqual(
diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzSuppressHydrationWarning-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzSuppressHydrationWarning-test.js index b77c88ca5081b..c53ddee3fb13b 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzSuppressHydrationWarning-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzSuppressHydrationWarning-test.js @@ -130,7 +130,7 @@ describe('ReactDOMFizzServerHydrationWarning', () => { : children; } - it('suppresses and fixes text mismatches with suppressHydrationWarning', async () => { + it('suppresses but does not fix text mismatches with suppressHydrationWarning', async () => { function App({isClient}) { return (
@@ -163,13 +163,13 @@ describe('ReactDOMFizzServerHydrationWarning', () => { // The text mismatch should be *silently* fixed. Even in production. expect(getVisibleChildren(container)).toEqual(
- Client Text - 2 + Server Text + 1
, ); }); - it('suppresses and fixes multiple text node mismatches with suppressHydrationWarning', async () => { + it('suppresses but does not fix multiple text node mismatches with suppressHydrationWarning', async () => { function App({isClient}) { return (
@@ -203,8 +203,8 @@ describe('ReactDOMFizzServerHydrationWarning', () => { expect(getVisibleChildren(container)).toEqual(
- {'Client1'} - {'Client2'} + {'Server1'} + {'Server2'}
, ); @@ -261,19 +261,17 @@ describe('ReactDOMFizzServerHydrationWarning', () => { ); }); - it('suppresses and fixes client-only single text node mismatches with suppressHydrationWarning', async () => { - function App({isClient}) { + it('suppresses but does not fix client-only single text node mismatches with suppressHydrationWarning', async () => { + function App({text}) { return (
- - {isClient ? 'Client' : null} - + {text}
); } await act(() => { const {pipe} = ReactDOMFizzServer.renderToPipeableStream( - , + , ); pipe(writable); }); @@ -282,7 +280,7 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
, ); - ReactDOMClient.hydrateRoot(container, , { + const root = ReactDOMClient.hydrateRoot(container, , { onRecoverableError(error) { Scheduler.log(error.message); }, @@ -290,7 +288,15 @@ describe('ReactDOMFizzServerHydrationWarning', () => { await waitForAll([]); expect(getVisibleChildren(container)).toEqual(
- {'Client'} + +
, + ); + // An update fixes it though. + root.render(); + await waitForAll([]); + expect(getVisibleChildren(container)).toEqual( +
+ Client 2
, ); }); @@ -495,7 +501,7 @@ describe('ReactDOMFizzServerHydrationWarning', () => { ); }); - it('suppresses and does not fix attribute mismatches with suppressHydrationWarning', async () => { + it('suppresses but does not fix attribute mismatches with suppressHydrationWarning', async () => { function App({isClient}) { return (
diff --git a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js index 2df936507724b..c3a6da112cb4c 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js @@ -5617,7 +5617,7 @@ background-color: green; ]); }); - // @gate enableFloat && enableHostSingletons && (enableClientRenderFallbackOnTextMismatch || !__DEV__) + // @gate enableFloat && enableHostSingletons && enableClientRenderFallbackOnTextMismatch it('can render a title before a singleton even if that singleton clears its contents', async () => { await actIntoEmptyDocument(() => { const {pipe} = renderToPipeableStream( diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.js b/packages/react-reconciler/src/ReactFiberHydrationContext.js index b6c7869e4596a..e8ce874e34edc 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.js @@ -728,6 +728,11 @@ function prepareToHydrateHostTextInstance(fiber: Fiber): boolean { isConcurrentMode, shouldWarnIfMismatchDev, ); + if (isConcurrentMode) { + // In concurrent mode we never update the mismatched text, + // even if the error was ignored. + return false; + } break; } case HostSingleton: @@ -747,6 +752,11 @@ function prepareToHydrateHostTextInstance(fiber: Fiber): boolean { isConcurrentMode, shouldWarnIfMismatchDev, ); + if (isConcurrentMode) { + // In concurrent mode we never update the mismatched text, + // even if the error was ignored. + return false; + } break; } }