From a934494976a04e511d5a42ce605ff1057bfa68d9 Mon Sep 17 00:00:00 2001 From: Ivan Babak Date: Fri, 8 Jun 2018 00:07:41 -0700 Subject: [PATCH] Refactor hydrate warnings with feedback from @KidkArolis, @mxstbr (#10085) - Change headline to contain only the tag names, not attributes and children. - Add newlines between headline, diff, and stack. Besides the above feedback: - Add comment nodes display in the diff. - Add weird node handling in the diff. --- .../src/__tests__/ReactMount-test.js | 161 +++++++++++++----- .../src/client/ReactDOMFiberComponent.js | 137 ++++++++++----- scripts/rollup/results.json | 22 +-- 3 files changed, 220 insertions(+), 100 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactMount-test.js b/packages/react-dom/src/__tests__/ReactMount-test.js index 7c7e1e2fcf3f8..2e3b700473e34 100644 --- a/packages/react-dom/src/__tests__/ReactMount-test.js +++ b/packages/react-dom/src/__tests__/ReactMount-test.js @@ -125,8 +125,11 @@ describe('ReactMount', () => { container.innerHTML = ReactDOMServer.renderToString(
) + ' '; expect(() => ReactDOM.hydrate(
, container)).toWarnDev( - "Warning: Did not expect server HTML to contain the text node {' '}" + - ' in
.', + "Warning: Did not expect server HTML to contain the text node {' '} in .\n\n" + + ' \n' + + '
\n' + + "- {' '}\n" + + ' \n', ); }); @@ -135,8 +138,11 @@ describe('ReactMount', () => { container.innerHTML = ' ' + ReactDOMServer.renderToString(
); expect(() => ReactDOM.hydrate(
, container)).toWarnDev( - "Warning: Did not expect server HTML to contain the text node {' '}" + - ' in
.', + "Warning: Did not expect server HTML to contain the text node {' '} in .\n\n" + + ' \n' + + "- {' '}\n" + + '
\n' + + ' \n', ); }); @@ -263,7 +269,7 @@ describe('ReactMount', () => { const div = document.createElement('div'); const markup = ReactDOMServer.renderToString( - nested{' '} + nested{' '}

children text

@@ -271,10 +277,14 @@ describe('ReactMount', () => { ); div.innerHTML = markup; + expect(div.outerHTML).toEqual( + '
nested

children text

', + ); + expect(() => ReactDOM.hydrate( - nested{' '} + nested{' '}
children text
@@ -282,14 +292,14 @@ describe('ReactMount', () => { div, ), ).toWarnDev( - "Warning: Expected server HTML to contain a matching
{['children ', …]}
" + - ' in
nested

children text

.\n' + + 'Warning: Expected server HTML to contain a matching
in
.\n\n' + '
\n' + " {'nested'}\n" + - " {' '}\n" + + ' \n' + + " {' '}\n" + '-

children text

\n' + "+
{['children ', …]}
\n" + - '
\n' + + '
\n\n' + ' in div (at **)\n' + ' in Component (at **)', ); @@ -307,7 +317,7 @@ describe('ReactMount', () => { const div = document.createElement('div'); const markup = ReactDOMServer.renderToString( - nested{' '} + nested{' '}

children text

@@ -315,10 +325,14 @@ describe('ReactMount', () => { ); div.innerHTML = markup; + expect(div.outerHTML).toEqual( + '
nested

children text

', + ); + expect(() => ReactDOM.hydrate( - nested{' '} + nested{' '}

children text

@@ -329,14 +343,14 @@ describe('ReactMount', () => { div, ), ).toWarnDev( - "Warning: Expected server HTML to contain a matching
{['children ', …]}
" + - ' in
nested

children text

.\n' + + 'Warning: Expected server HTML to contain a matching
in
.\n\n' + '
\n' + " {'nested'}\n" + - " {' '}\n" + + ' \n' + + " {' '}\n" + '

children text

\n' + "+
{['children ', …]}
\n" + - '
\n' + + '
\n\n' + ' in div (at **)\n' + ' in Component (at **)', ); @@ -483,15 +497,13 @@ describe('ReactMount', () => { div, ), ).toWarnDev( - 'Warning: Did not expect server HTML to contain a
' + - ' in
SSRMismatchTest first text' + - '

SSRMismatchTest second text
.\n' + + 'Warning: Did not expect server HTML to contain a
in
.\n\n' + '
\n' + " {'SSRMismatchTest first text'}\n" + '
\n' + '-
\n' + " {'SSRMismatchTest second text'}\n" + - '
', + '
\n', ); }); @@ -520,9 +532,7 @@ describe('ReactMount', () => { div, ), ).toWarnDev( - 'Warning: Did not expect server HTML to contain a
SSRMismatchTest default text
' + - ' in
SSRMismatchTest default text
' + - '
.\n' + + 'Warning: Did not expect server HTML to contain a
in
.\n\n' + '
\n' + '-
SSRMismatchTest default text
\n' + ' \n' + @@ -531,7 +541,7 @@ describe('ReactMount', () => { '
\n' + '
\n' + '
\n' + - '
\n' + + '
\n\n' + ' in span (at **)\n' + ' in div (at **)', ); @@ -556,9 +566,7 @@ describe('ReactMount', () => { expect(() => ReactDOM.hydrate([
, 'SSRMismatchTest default text'], div), ).toWarnDev( - "Warning: Did not expect server HTML to contain the text node {'SSRMismatchTest server text'}" + - ' in
SSRMismatchTest server text' + - '
SSRMismatchTest default text
<…
.\n' + + "Warning: Did not expect server HTML to contain the text node {'SSRMismatchTest server text'} in
.\n\n" + '
\n' + "- {'SSRMismatchTest server text'}\n" + '
\n' + @@ -568,7 +576,7 @@ describe('ReactMount', () => { '
\n' + '
\n' + '
\n' + - '
\n' + + '
\n\n' + ' in br (at **)', ); }); @@ -598,9 +606,7 @@ describe('ReactMount', () => { div, ), ).toWarnDev( - "Warning: Did not expect server HTML to contain the text node {'SSRMismatchTest server text'}" + - ' in
SSRMismatchTest server text' + - '
.\n' + + "Warning: Did not expect server HTML to contain the text node {'SSRMismatchTest server text'} in
.\n\n" + '
\n' + "- {'SSRMismatchTest server text'}\n" + ' \n' + @@ -609,7 +615,7 @@ describe('ReactMount', () => { '
\n' + '
\n' + '
\n' + - '
\n' + + '
\n\n' + ' in span (at **)\n' + ' in div (at **)', ); @@ -625,12 +631,11 @@ describe('ReactMount', () => { expect(() => ReactDOM.hydrate(SSRMismatchTest default text, div), ).toWarnDev( - 'Warning: Expected server HTML to contain a matching SSRMismatchTest default text' + - ' in
SSRMismatchTest default text
.\n' + + 'Warning: Expected server HTML to contain a matching in
.\n\n' + '
\n' + "- {'SSRMismatchTest default text'}\n" + '+ SSRMismatchTest default text\n' + - '
\n' + + '
\n\n' + ' in span (at **)', ); }); @@ -654,12 +659,11 @@ describe('ReactMount', () => { div, ), ).toWarnDev( - 'Warning: Expected server HTML to contain a matching

SSRMismatchTest default text

' + - ' in
SSRMismatchTest default text
.\n' + + 'Warning: Expected server HTML to contain a matching

in

.\n\n' + '
\n' + '- SSRMismatchTest default text\n' + '+

SSRMismatchTest default text

\n' + - '
\n' + + '
\n\n' + ' in p (at **)\n' + ' in div (at **)', ); @@ -677,8 +681,12 @@ describe('ReactMount', () => { expect(() => ReactDOM.hydrate('SSRMismatchTest default text', div), ).toWarnDev( - "Warning: Expected server HTML to contain a matching text node for {'SSRMismatchTest default text'}" + - ' in
SSRMismatchTest default text
.', + 'Warning: Expected server HTML to contain a matching text node' + + " for {'SSRMismatchTest default text'} in
.\n\n" + + '
\n' + + '- SSRMismatchTest default text\n' + + "+ {'SSRMismatchTest default text'}\n" + + '
\n', ); }); @@ -709,9 +717,8 @@ describe('ReactMount', () => { div, ), ).toWarnDev( - "Warning: Expected server HTML to contain a matching text node for {'SSRMismatchTest client text'}" + - ' in
' + - '
.\n' + + 'Warning: Expected server HTML to contain a matching text node' + + " for {'SSRMismatchTest client text'} in
.\n\n" + '
\n' + ' \n' + '- \n' + @@ -721,7 +728,73 @@ describe('ReactMount', () => { '
\n' + '
\n' + '
\n' + - '
\n' + + '
\n\n' + + ' in div (at **)', + ); + }); + + it('should warn when hydrate inserts an element after a comment node', () => { + const div = document.createElement('div'); + div.innerHTML = '
'; + + expect(() => + ReactDOM.hydrate( +
+ +
, + div, + ), + ).toWarnDev( + 'Warning: Expected server HTML to contain a matching in
.\n\n' + + '
\n' + + ' \n' + + '+ \n' + + '
\n\n' + + ' in span (at **)\n' + + ' in div (at **)', + ); + }); + + it('should warn when hydrate replaces an element after a comment node', () => { + const div = document.createElement('div'); + div.innerHTML = '
'; + + expect(() => + ReactDOM.hydrate( +
+ +
+
, + div, + ), + ).toWarnDev( + 'Warning: Expected server HTML to contain a matching in
.\n\n' + + '
\n' + + ' \n' + + '-
\n' + + '+ \n' + + '
\n\n' + + ' in span (at **)\n' + + ' in div (at **)', + ); + }); + + it('should warn when hydrate inserts an element after a node that is not an element, text, or comment', () => { + // This is an artificial test case to check how we print weird nodes if they somehow end up in the DOM. + const xml = document.createElement('xml'); + xml.appendChild( + document.createProcessingInstruction( + 'dom-processing-instruction', + 'content', + ), + ); + + expect(() => ReactDOM.hydrate(
, xml)).toWarnDev( + 'Warning: Expected server HTML to contain a matching
in .\n\n' + + ' \n' + + ' \n' + + '+
\n' + + ' \n\n' + ' in div (at **)', ); }); diff --git a/packages/react-dom/src/client/ReactDOMFiberComponent.js b/packages/react-dom/src/client/ReactDOMFiberComponent.js index 2f77cf009e6d9..aae8d377c994e 100644 --- a/packages/react-dom/src/client/ReactDOMFiberComponent.js +++ b/packages/react-dom/src/client/ReactDOMFiberComponent.js @@ -71,7 +71,9 @@ const HTML = '__html'; const {html: HTML_NAMESPACE} = Namespaces; let getStack = emptyFunction.thatReturns(''); -let getNodeSignature = emptyFunction.thatReturns('<…>'); +let getTextContentSignature = emptyFunction.thatReturns(''); +let getNodeSignatureForDiff = emptyFunction.thatReturns('<…>'); +let getNodeSignatureForMessage = emptyFunction.thatReturns('<…>'); let getTagWithPropsSignature = emptyFunction.thatReturns('<…>'); let getNodeSurroundingsAndDiff = emptyFunction.thatReturns(''); let findNodeIndex = emptyFunction.thatReturns(-1); @@ -91,46 +93,88 @@ let normalizeHTML; if (__DEV__) { getStack = getCurrentFiberStackAddendum; - getNodeSignature = function(node: Node, openTagOnly = false) { - // TODO: How do we want to print quotes inside quotes? - // TODO: How do we want to print special characters such as  ? + const NODE_CONTENT_CLIP_LENGTH = 100; + + const clipStringWithEllipsis = function(str: string, clipAtLength: number) { + return ( + str.substring(0, clipAtLength) + (str.length > clipAtLength ? '…' : '') + ); + }; + + getTextContentSignature = function(textContent: string) { + // TODO: How do we want to print quotes and other special characters inside string quotes? + return ( + "{'" + + clipStringWithEllipsis(textContent, NODE_CONTENT_CLIP_LENGTH) + + "'}" + ); + }; + + const getSpecialNodeSignature = function(node: Node) { const nodeType = node.nodeType; if (nodeType === COMMENT_NODE) { - return ''; + return ( + '' + ); } if (nodeType === TEXT_NODE) { - return "{'" + node.textContent + "'}"; + return getTextContentSignature(node.textContent); + } + // In normal circumstances, we should only get COMMENT_NODE or TEXT_NODE here. + // But while looping over childNodes in getNodeSignatureForDiff, something else can show up. + // Below is a safety net if we'll have to display a Node that's not an element, text, or comment. + // For example, a PROCESSING_INSTRUCTION_NODE. + return ( + '' + ); + }; + + const normalizeNodeName = function(nodeName: string) { + return nodeName.toLowerCase(); + }; + + getNodeSignatureForMessage = function(node: Node) { + if (node.nodeType !== ELEMENT_NODE) { + return getSpecialNodeSignature(node); } - if (nodeType !== ELEMENT_NODE) { - return node.nodeName; + return '<' + normalizeNodeName(node.nodeName) + '>'; + }; + + getNodeSignatureForDiff = function(node: Node, openTagOnly = false) { + if (node.nodeType !== ELEMENT_NODE) { + return getSpecialNodeSignature(node); } - const tag = node.nodeName.toLowerCase(); - let ret = '<' + tag; + const tagName = normalizeNodeName(node.nodeName); + let ret = '<' + tagName; if (node instanceof Element) { const attrs = node.attributes; const ic = attrs.length; for (let i = 0; i < ic; ++i) { + // TODO: Should we process the value with clipStringWithEllipsis? + // TODO: How do we want to print quotes and other HTML special characters inside attribute quotes? ret += ' ' + attrs[i].name + '="' + attrs[i].value + '"'; } } if (openTagOnly) { ret += '>'; } else { - const childrenString = + const childrenContent = node instanceof Element ? node.innerHTML : node.textContent; - if (omittedCloseTags.hasOwnProperty(tag)) { + if (omittedCloseTags.hasOwnProperty(tagName)) { ret += ' />'; } else { - if (childrenString) { - ret += - '>' + - childrenString.substring(0, 100) + - (childrenString.length > 100 ? '…' : '') + - ''; + ret += + '>' + + clipStringWithEllipsis(childrenContent, NODE_CONTENT_CLIP_LENGTH) + + ''; } } return ret; @@ -215,12 +259,13 @@ if (__DEV__) { insertProps: Object | null, insertText: string | null, ) { - let ret = ''; + // getStack prepends '\n' to its output, so making getNodeSurroundingsAndDiff do the same. + let ret = '\n'; const INDENT = ' '; const DIFF_ADDED = '\n+ '; const DIFF_REMOVED = '\n- '; const DIFF_UNCHANGED = '\n '; - ret += DIFF_UNCHANGED + getNodeSignature(parentNode, true); + ret += DIFF_UNCHANGED + getNodeSignatureForDiff(parentNode, true); let inserted = false; const insert = () => { if (!inserted) { @@ -232,36 +277,38 @@ if (__DEV__) { getTagWithPropsSignature(insertTag, insertProps || {}); } else if (typeof insertText === 'string') { inserted = true; - ret += DIFF_ADDED + INDENT + "{'" + insertText + "'}"; + ret += DIFF_ADDED + INDENT + getTextContentSignature(insertText); } } }; const childNodes = parentNode.childNodes; const ic = childNodes.length; - let skipped = 0; + let hydrationSkippedCount = 0; for (let i = 0; i < ic; ++i) { const node = childNodes[i]; - // TODO: Copy-paste condition from `getNextHydratableSibling`, reuse? + // TODO: Below is a copy-paste of the condition from `getNextHydratableSibling`, how to not repeat ourselves? if ( node && node.nodeType !== ELEMENT_NODE && node.nodeType !== TEXT_NODE ) { - ++skipped; + // Hydration skips other nodeTypes, like COMMENT_NODE, but we display them to provide more contextual info. + ret += DIFF_UNCHANGED + INDENT + getNodeSignatureForDiff(node); + ++hydrationSkippedCount; continue; } - if (i - skipped === deletedIndex) { - ret += DIFF_REMOVED + INDENT + getNodeSignature(node); + if (i - hydrationSkippedCount === deletedIndex) { + ret += DIFF_REMOVED + INDENT + getNodeSignatureForDiff(node); } else { - ret += DIFF_UNCHANGED + INDENT + getNodeSignature(node); + ret += DIFF_UNCHANGED + INDENT + getNodeSignatureForDiff(node); } - if (i - skipped === insertedIndex) { + if (i - hydrationSkippedCount === insertedIndex) { insert(); } } insert(); // TODO: Cannot tell if more sibling React elements were expected to be hydrated after the current one. - ret += DIFF_UNCHANGED + ''; + ret += DIFF_UNCHANGED + ''; return ret; }; @@ -1323,9 +1370,9 @@ export function warnForDeletedHydratableElement( didWarnInvalidHydration = true; warning( false, - 'Did not expect server HTML to contain a %s in %s.%s%s', - getNodeSignature(child), - getNodeSignature(parentNode), + 'Did not expect server HTML to contain a %s in %s.%s\n%s', + getNodeSignatureForMessage(child), + getNodeSignatureForMessage(parentNode), getNodeSurroundingsAndDiff( parentNode, findNodeIndex(parentNode.childNodes, child), @@ -1350,9 +1397,9 @@ export function warnForDeletedHydratableText( didWarnInvalidHydration = true; warning( false, - "Did not expect server HTML to contain the text node {'%s'} in %s.%s%s", - child.nodeValue, - getNodeSignature(parentNode), + 'Did not expect server HTML to contain the text node %s in %s.%s\n%s', + getTextContentSignature(child.nodeValue), + getNodeSignatureForMessage(parentNode), getNodeSurroundingsAndDiff( parentNode, findNodeIndex(parentNode.childNodes, child), @@ -1380,9 +1427,9 @@ export function warnForInsertedHydratedElement( didWarnInvalidHydration = true; warning( false, - 'Expected server HTML to contain a matching %s in %s.%s%s', - getTagWithPropsSignature(tag, props), - getNodeSignature(parentNode), + 'Expected server HTML to contain a matching <%s> in %s.%s\n%s', + tag, + getNodeSignatureForMessage(parentNode), getNodeSurroundingsAndDiff( parentNode, isReplaced ? index : -1, @@ -1416,9 +1463,9 @@ export function warnForInsertedHydratedText( didWarnInvalidHydration = true; warning( false, - "Expected server HTML to contain a matching text node for {'%s'} in %s.%s%s", - text, - getNodeSignature(parentNode), + 'Expected server HTML to contain a matching text node for %s in %s.%s\n%s', + getTextContentSignature(text), + getNodeSignatureForMessage(parentNode), getNodeSurroundingsAndDiff( parentNode, isReplaced ? index : -1, diff --git a/scripts/rollup/results.json b/scripts/rollup/results.json index 41faf3fcb0cae..cdd73ef1c07fe 100644 --- a/scripts/rollup/results.json +++ b/scripts/rollup/results.json @@ -46,29 +46,29 @@ "filename": "react-dom.development.js", "bundleType": "UMD_DEV", "packageName": "react-dom", - "size": 647324, - "gzip": 150750 + "size": 649236, + "gzip": 151210 }, { "filename": "react-dom.production.min.js", "bundleType": "UMD_PROD", "packageName": "react-dom", - "size": 96522, - "gzip": 31260 + "size": 96566, + "gzip": 31261 }, { "filename": "react-dom.development.js", "bundleType": "NODE_DEV", "packageName": "react-dom", - "size": 631307, - "gzip": 146658 + "size": 633215, + "gzip": 147124 }, { "filename": "react-dom.production.min.js", "bundleType": "NODE_PROD", "packageName": "react-dom", - "size": 94973, - "gzip": 30207 + "size": 95017, + "gzip": 30210 }, { "filename": "ReactDOM-dev.js", @@ -515,14 +515,14 @@ "filename": "ReactDOM-dev.js", "bundleType": "FB_WWW_DEV", "packageName": "react-dom", - "size": 641820, - "gzip": 146072 + "size": 643837, + "gzip": 146553 }, { "filename": "ReactDOM-prod.js", "bundleType": "FB_WWW_PROD", "packageName": "react-dom", - "size": 275786, + "size": 275856, "gzip": 51804 }, {