From 2d3f81bb6a650386832d885d7b63a7d0d517ba15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Wed, 10 Jul 2024 12:17:13 -0400 Subject: [PATCH] Format DOM Nesting Warning as Diff View + An Additional Log for Stack Trace (#30302) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently we're printing parent stacks at the end of DOM nesting even with owner stacks enabled. That's because the context of parent tree is relevant for determining why two things are nested. It might not be sufficient to see the owner stack alone. I'm trying to get rid of parent stacks and rely on more of the plain owner stacks or ideally console.createTask. These are generally better anyway since the exact line for creating the JSX is available. It also lets you find a parent stack frame that is most relevant e.g. if it's hidden inside internals. For DOM nesting there's really only two stacks that are relevant. The creation of the parent and the creation of the child. Sometimes they're close enough to be the same thing. Such as for parents that can't have text children or when the ancestor is the direct parent created at the same place (same owner). Sometimes they're far apart. In this case I add a second console.error within the context of the ancestor. That way the second stack trace can be used to read the stack trace for where it was created. To preserve some parent context I now print the parent stack in a diff view format using the logic from hydration diffs. This includes some siblings and props for context. Screenshot 2024-07-10 at 12 21 38 AM Text Nodes: Screenshot 2024-07-10 at 12 37 40 AM --------- Co-authored-by: tjallingt --- .../src/client/validateDOMNesting.js | 142 +++++++-- .../src/__tests__/ReactDOMComponent-test.js | 275 +++++++++++++----- .../src/__tests__/ReactDOMForm-test.js | 12 +- .../src/__tests__/ReactDOMOption-test.js | 26 +- .../src/__tests__/validateDOMNesting-test.js | 71 +++-- .../react-reconciler/src/ReactChildFiber.js | 33 +++ .../react-reconciler/src/ReactCurrentFiber.js | 11 - .../src/ReactFiberHydrationDiffs.js | 22 +- 8 files changed, 453 insertions(+), 139 deletions(-) diff --git a/packages/react-dom-bindings/src/client/validateDOMNesting.js b/packages/react-dom-bindings/src/client/validateDOMNesting.js index 2f192cd18bd94..49d8b5c158f62 100644 --- a/packages/react-dom-bindings/src/client/validateDOMNesting.js +++ b/packages/react-dom-bindings/src/client/validateDOMNesting.js @@ -7,7 +7,54 @@ * @flow */ -import {getCurrentParentStackInDev} from 'react-reconciler/src/ReactCurrentFiber'; +import type {Fiber} from 'react-reconciler/src/ReactInternalTypes'; +import type {HydrationDiffNode} from 'react-reconciler/src/ReactFiberHydrationDiffs'; + +import {enableOwnerStacks} from 'shared/ReactFeatureFlags'; + +import { + current, + runWithFiberInDEV, +} from 'react-reconciler/src/ReactCurrentFiber'; +import { + HostComponent, + HostHoistable, + HostSingleton, + HostText, +} from 'react-reconciler/src/ReactWorkTags'; + +import {describeDiff} from 'react-reconciler/src/ReactFiberHydrationDiffs'; + +function describeAncestors( + ancestor: Fiber, + child: Fiber, + props: null | {children: null}, +): string { + let fiber: null | Fiber = child; + let node: null | HydrationDiffNode = null; + let distanceFromLeaf = 0; + while (fiber) { + if (fiber === ancestor) { + distanceFromLeaf = 0; + } + node = { + fiber: fiber, + children: node !== null ? [node] : [], + serverProps: + fiber === child ? props : fiber === ancestor ? null : undefined, + serverTail: [], + distanceFromLeaf: distanceFromLeaf, + }; + distanceFromLeaf++; + fiber = fiber.return; + } + if (node !== null) { + // Describe the node using the hydration diff logic. + // Replace + with - to mark ancestor and child. It's kind of arbitrary. + return describeDiff(node).replaceAll(/^[+-]/gm, '>'); + } + return ''; +} type Info = {tag: string}; export type AncestorInfoDev = { @@ -440,6 +487,21 @@ function findInvalidAncestorForTag( const didWarn: {[string]: boolean} = {}; +function findAncestor(parent: null | Fiber, tagName: string): null | Fiber { + while (parent) { + switch (parent.tag) { + case HostComponent: + case HostHoistable: + case HostSingleton: + if (parent.type === tagName) { + return parent; + } + } + parent = parent.return; + } + return null; +} + function validateDOMNesting( childTag: string, ancestorInfo: AncestorInfoDev, @@ -470,6 +532,14 @@ function validateDOMNesting( } didWarn[warnKey] = true; + const child = current; + const ancestor = child ? findAncestor(child.return, ancestorTag) : null; + + const ancestorDescription = + child !== null && ancestor !== null + ? describeAncestors(ancestor, child, null) + : ''; + const tagDisplayName = '<' + childTag + '>'; if (invalidParent) { let info = ''; @@ -478,33 +548,45 @@ function validateDOMNesting( ' Add a , or to your code to match the DOM tree generated by ' + 'the browser.'; } - // Don't transform into consoleWithStackDev here because we add a manual stack. - // We use the parent stack here instead of the owner stack because the parent - // stack has more useful context for nesting. - // TODO: Format this as a linkified "diff view" with props instead of - // a stack trace since the stack trace format is now for owner stacks. - console['error']( + console.error( 'In HTML, %s cannot be a child of <%s>.%s\n' + 'This will cause a hydration error.%s', tagDisplayName, ancestorTag, info, - getCurrentParentStackInDev(), + ancestorDescription, ); } else { - // Don't transform into consoleWithStackDev here because we add a manual stack. - // We use the parent stack here instead of the owner stack because the parent - // stack has more useful context for nesting. - // TODO: Format this as a linkified "diff view" with props instead of - // a stack trace since the stack trace format is now for owner stacks. - console['error']( + console.error( 'In HTML, %s cannot be a descendant of <%s>.\n' + 'This will cause a hydration error.%s', tagDisplayName, ancestorTag, - getCurrentParentStackInDev(), + ancestorDescription, ); } + if (enableOwnerStacks && child) { + // For debugging purposes find the nearest ancestor that caused the issue. + // The stack trace of this ancestor can be useful to find the cause. + // If the parent is a direct parent in the same owner, we don't bother. + const parent = child.return; + if ( + ancestor !== null && + parent !== null && + (ancestor !== parent || parent._debugOwner !== child._debugOwner) + ) { + runWithFiberInDEV(ancestor, () => { + console.error( + // We repeat some context because this log might be taken out of context + // such as in React DevTools or grouped server logs. + '<%s> cannot contain a nested %s.\n' + + 'See this log for the ancestor stack trace.', + ancestorTag, + tagDisplayName, + ); + }); + } + } return false; } return true; @@ -522,31 +604,33 @@ function validateTextNesting(childText: string, parentTag: string): boolean { } didWarn[warnKey] = true; + const child = current; + const ancestor = child ? findAncestor(child, parentTag) : null; + + const ancestorDescription = + child !== null && ancestor !== null + ? describeAncestors( + ancestor, + child, + child.tag !== HostText ? {children: null} : null, + ) + : ''; + if (/\S/.test(childText)) { - // Don't transform into consoleWithStackDev here because we add a manual stack. - // We use the parent stack here instead of the owner stack because the parent - // stack has more useful context for nesting. - // TODO: Format this as a linkified "diff view" with props instead of - // a stack trace since the stack trace format is now for owner stacks. - console['error']( + console.error( 'In HTML, text nodes cannot be a child of <%s>.\n' + 'This will cause a hydration error.%s', parentTag, - getCurrentParentStackInDev(), + ancestorDescription, ); } else { - // Don't transform into consoleWithStackDev here because we add a manual stack. - // We use the parent stack here instead of the owner stack because the parent - // stack has more useful context for nesting. - // TODO: Format this as a linkified "diff view" with props instead of - // a stack trace since the stack trace format is now for owner stacks. - console['error']( + console.error( 'In HTML, whitespace text nodes cannot be a child of <%s>. ' + "Make sure you don't have any extra whitespace between tags on " + 'each line of your source code.\n' + 'This will cause a hydration error.%s', parentTag, - getCurrentParentStackInDev(), + ancestorDescription, ); } return false; diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js index 6e02e17ece1a7..d37a4ecba6dc6 100644 --- a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js @@ -2193,13 +2193,18 @@ describe('ReactDOMComponent', () => { , ); }); - }).toErrorDev([ - 'In HTML, cannot be a child of ' + - '
.\n' + - 'This will cause a hydration error.' + + }).toErrorDev( + 'In HTML, cannot be a child of
.\n' + + 'This will cause a hydration error.\n' + + '\n' + + '>
\n' + + '> \n' + + ' ...\n' + '\n in tr (at **)' + - '\n in div (at **)', - ]); + (gate(flags => flags.enableOwnerStacks) + ? '' + : '\n in div (at **)'), + ); }); it('warns on invalid nesting at root', async () => { @@ -2215,12 +2220,13 @@ describe('ReactDOMComponent', () => { ); }); }).toErrorDev( - 'In HTML,

cannot be a descendant ' + - 'of

.\n' + + 'In HTML,

cannot be a descendant of

.\n' + 'This will cause a hydration error.' + // There is no outer `p` here because root container is not part of the stack. '\n in p (at **)' + - '\n in span (at **)', + (gate(flags => flags.enableOwnerStacks) + ? '' + : '\n in span (at **)'), ); }); @@ -2248,29 +2254,90 @@ describe('ReactDOMComponent', () => { await act(() => { root.render(); }); - }).toErrorDev([ - 'In HTML, cannot be a child of ' + - '. Add a , or to your code to match the DOM tree generated ' + - 'by the browser.\n' + - 'This will cause a hydration error.' + - '\n in tr (at **)' + - '\n in Row (at **)' + - '\n in table (at **)' + - '\n in Foo (at **)', - 'In HTML, text nodes cannot be a ' + - 'child of .\n' + - 'This will cause a hydration error.' + - '\n in tr (at **)' + - '\n in Row (at **)' + - '\n in table (at **)' + - '\n in Foo (at **)', - 'In HTML, whitespace text nodes cannot ' + - "be a child of
. Make sure you don't have any extra " + - 'whitespace between tags on each line of your source code.\n' + - 'This will cause a hydration error.' + - '\n in table (at **)' + - '\n in Foo (at **)', - ]); + }).toErrorDev( + gate(flags => flags.enableOwnerStacks) + ? [ + 'In HTML, cannot be a child of ' + + '
. Add a , or to your code to match the DOM tree generated ' + + 'by the browser.\n' + + 'This will cause a hydration error.\n' + + '\n' + + ' \n' + + '>
\n' + + ' \n' + + '> \n' + + ' ...\n' + + '\n in tr (at **)' + + '\n in Row (at **)', + '
cannot contain a nested .\nSee this log for the ancestor stack trace.' + + '\n in table (at **)' + + '\n in Foo (at **)', + 'In HTML, text nodes cannot be a ' + + 'child of .\n' + + 'This will cause a hydration error.\n' + + '\n' + + ' \n' + + '
\n' + + ' \n' + + ' \n' + + '> x\n' + + ' ...\n' + + '\n in tr (at **)' + + '\n in Row (at **)', + 'In HTML, whitespace text nodes cannot ' + + "be a child of
. Make sure you don't have any extra " + + 'whitespace between tags on each line of your source code.\n' + + 'This will cause a hydration error.\n' + + '\n' + + ' \n' + + '>
\n' + + ' \n' + + '> {" "}\n' + + '\n in table (at **)' + + '\n in Foo (at **)', + ] + : [ + 'In HTML, cannot be a child of ' + + '
. Add a , or to your code to match the DOM tree generated ' + + 'by the browser.\n' + + 'This will cause a hydration error.\n' + + '\n' + + ' \n' + + '>
\n' + + ' \n' + + '> \n' + + ' ...\n' + + '\n in tr (at **)' + + '\n in Row (at **)' + + '\n in table (at **)' + + '\n in Foo (at **)', + 'In HTML, text nodes cannot be a ' + + 'child of .\n' + + 'This will cause a hydration error.\n' + + '\n' + + ' \n' + + '
\n' + + ' \n' + + ' \n' + + '> x\n' + + ' ...\n' + + '\n in tr (at **)' + + '\n in Row (at **)' + + '\n in table (at **)' + + '\n in Foo (at **)', + 'In HTML, whitespace text nodes cannot ' + + "be a child of
. Make sure you don't have any extra " + + 'whitespace between tags on each line of your source code.\n' + + 'This will cause a hydration error.\n' + + '\n' + + ' \n' + + '>
\n' + + ' \n' + + '> {" "}\n' + + '\n in table (at **)' + + '\n in Foo (at **)', + ], + ); }); it('warns nicely for updating table rows to use text', async () => { @@ -2297,7 +2364,11 @@ describe('ReactDOMComponent', () => { 'In HTML, whitespace text nodes cannot ' + "be a child of
. Make sure you don't have any extra " + 'whitespace between tags on each line of your source code.\n' + - 'This will cause a hydration error.' + + 'This will cause a hydration error.\n' + + '\n' + + ' \n' + + '
\n' + + '> {" "}\n' + '\n in table (at **)' + '\n in Foo (at **)', ]); @@ -2325,12 +2396,21 @@ describe('ReactDOMComponent', () => { }).toErrorDev([ 'In HTML, text nodes cannot be a ' + 'child of .\n' + - 'This will cause a hydration error.' + + 'This will cause a hydration error.\n' + + '\n' + + ' \n' + + '
\n' + + ' \n' + + ' \n' + + ' \n' + + '> text\n' + '\n in tr (at **)' + '\n in Row (at **)' + - '\n in tbody (at **)' + - '\n in table (at **)' + - '\n in Foo (at **)', + (gate(flags => flags.enableOwnerStacks) + ? '' + : '\n in tbody (at **)' + + '\n in table (at **)' + + '\n in Foo (at **)'), ]); }); @@ -2359,11 +2439,21 @@ describe('ReactDOMComponent', () => { root.render(); }); }).toErrorDev( - '\n in tr (at **)' + - '\n in Row (at **)' + - '\n in FancyRow (at **)' + - '\n in table (at **)' + - '\n in Viz1 (at **)', + gate(flags => flags.enableOwnerStacks) + ? [ + '\n in tr (at **)' + + '\n in Row (at **)' + + '\n in FancyRow (at **)' + + '\n in Viz1 (at **)', + '\n in table (at **)' + '\n in Viz1 (at **)', + ] + : [ + '\n in tr (at **)' + + '\n in Row (at **)' + + '\n in FancyRow (at **)' + + '\n in table (at **)' + + '\n in Viz1 (at **)', + ], ); }); @@ -2405,13 +2495,26 @@ describe('ReactDOMComponent', () => { root.render(); }); }).toErrorDev( - '\n in tr (at **)' + - '\n in Row (at **)' + - '\n in FancyRow (at **)' + - '\n in table (at **)' + - '\n in Table (at **)' + - '\n in FancyTable (at **)' + - '\n in Viz2 (at **)', + gate(flags => flags.enableOwnerStacks) + ? [ + '\n in tr (at **)' + + '\n in Row (at **)' + + '\n in FancyRow (at **)' + + '\n in Viz2 (at **)', + '\n in table (at **)' + + '\n in Table (at **)' + + '\n in FancyTable (at **)' + + '\n in Viz2 (at **)', + ] + : [ + '\n in tr (at **)' + + '\n in Row (at **)' + + '\n in FancyRow (at **)' + + '\n in table (at **)' + + '\n in Table (at **)' + + '\n in FancyTable (at **)' + + '\n in Viz2 (at **)', + ], ); }); @@ -2446,12 +2549,23 @@ describe('ReactDOMComponent', () => { ); }); }).toErrorDev( - '\n in tr (at **)' + - '\n in Row (at **)' + - '\n in FancyRow (at **)' + - '\n in table (at **)' + - '\n in Table (at **)' + - '\n in FancyTable (at **)', + gate(flags => flags.enableOwnerStacks) + ? [ + '\n in tr (at **)' + + '\n in Row (at **)' + + '\n in FancyRow (at **)', + '\n in table (at **)' + + '\n in Table (at **)' + + '\n in FancyTable (at **)', + ] + : [ + '\n in tr (at **)' + + '\n in Row (at **)' + + '\n in FancyRow (at **)' + + '\n in table (at **)' + + '\n in Table (at **)' + + '\n in FancyTable (at **)', + ], ); }); @@ -2475,10 +2589,19 @@ describe('ReactDOMComponent', () => { ); }); }).toErrorDev( - '\n in tr (at **)' + - '\n in Row (at **)' + - '\n in FancyRow (at **)' + - '\n in table (at **)', + gate(flags => flags.enableOwnerStacks) + ? [ + '\n in tr (at **)' + + '\n in Row (at **)' + + '\n in FancyRow (at **)', + '\n in table (at **)', + ] + : [ + '\n in tr (at **)' + + '\n in Row (at **)' + + '\n in FancyRow (at **)' + + '\n in table (at **)', + ], ); }); @@ -2506,10 +2629,19 @@ describe('ReactDOMComponent', () => { ); }); }).toErrorDev( - '\n in tr (at **)' + - '\n in table (at **)' + - '\n in Table (at **)' + - '\n in FancyTable (at **)', + gate(flags => flags.enableOwnerStacks) + ? [ + '\n in tr (at **)', + '\n in table (at **)' + + '\n in Table (at **)' + + '\n in FancyTable (at **)', + ] + : [ + '\n in tr (at **)' + + '\n in table (at **)' + + '\n in Table (at **)' + + '\n in FancyTable (at **)', + ], ); class Link extends React.Component { @@ -2531,11 +2663,18 @@ describe('ReactDOMComponent', () => { ); }); }).toErrorDev( - '\n in a (at **)' + - '\n in Link (at **)' + - '\n in div (at **)' + - '\n in a (at **)' + - '\n in Link (at **)', + gate(flags => flags.enableOwnerStacks) + ? [ + '\n in a (at **)' + '\n in Link (at **)', + '\n in a (at **)' + '\n in Link (at **)', + ] + : [ + '\n in a (at **)' + + '\n in Link (at **)' + + '\n in div (at **)' + + '\n in a (at **)' + + '\n in Link (at **)', + ], ); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMForm-test.js b/packages/react-dom/src/__tests__/ReactDOMForm-test.js index 4c3ebecccb1e9..7ba4bea06bacf 100644 --- a/packages/react-dom/src/__tests__/ReactDOMForm-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMForm-test.js @@ -385,12 +385,16 @@ describe('ReactDOMForm', () => { , ); }); - }).toErrorDev([ + }).toErrorDev( 'In HTML,
cannot be a descendant of .\n' + - 'This will cause a hydration error.' + + 'This will cause a hydration error.\n' + + '\n' + + '> \n' + + ' \n' + + '> \n' + '\n in form (at **)' + - '\n in form (at **)', - ]); + (gate(flags => flags.enableOwnerStacks) ? '' : '\n in form (at **)'), + ); await submit(ref.current); diff --git a/packages/react-dom/src/__tests__/ReactDOMOption-test.js b/packages/react-dom/src/__tests__/ReactDOMOption-test.js index dab7f69b27e22..ce5e3c65bcfdb 100644 --- a/packages/react-dom/src/__tests__/ReactDOMOption-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMOption-test.js @@ -53,8 +53,15 @@ describe('ReactDOMOption', () => { }).toErrorDev( 'In HTML,
cannot be a child of