From 144bc713aa9a4f572c569a50d7babd3ef18c6b3c Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 14 Mar 2023 23:07:51 -0400 Subject: [PATCH] Don't "fix up" mismatched text content with suppressedHydrationWarning --- .../src/client/ReactDOMComponent.js | 353 +++++++++--------- ...actDOMFizzSuppressHydrationWarning-test.js | 36 +- .../src/ReactFiberHydrationContext.js | 10 + 3 files changed, 203 insertions(+), 196 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js index 037d787197c85..74e0c9a706c16 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -883,11 +883,9 @@ export function diffHydratedProperties( shouldWarnDev: boolean, parentNamespaceDev: string, ): null | Array { - let isCustomComponentTag; let extraAttributeNames: Set; if (__DEV__) { - isCustomComponentTag = isCustomComponent(tag, rawProps); validatePropertiesInDevelopment(tag, rawProps); } @@ -953,6 +951,10 @@ export function diffHydratedProperties( break; } + if (rawProps.hasOwnProperty('onScroll')) { + listenToNonDelegatedEvent('scroll', domElement); + } + assertValidProps(tag, rawProps); if (__DEV__) { @@ -978,207 +980,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__/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-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; } }