Skip to content

Commit

Permalink
Format DOM Nesting Warning as Diff View + An Additional Log for Stack…
Browse files Browse the repository at this point in the history 
… Trace (facebook#30302)

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.

<img width="756" alt="Screenshot 2024-07-10 at 12 21 38 AM"
src="https://github.com/facebook/react/assets/63648/0843133d-cc7a-4ecc-91c0-f46ae8e99f20">

Text Nodes:

<img width="749" alt="Screenshot 2024-07-10 at 12 37 40 AM"
src="https://github.com/facebook/react/assets/63648/ee377d82-54ee-450a-99d1-fcc3ef290d59">

---------

Co-authored-by: tjallingt <tjallingt@gmail.com>
  • Loading branch information
@sebmarkbage @tjallingt
sebmarkbage and tjallingt authored Jul 10, 2024
1 parent 378b305 commit 2d3f81b
Show file tree
Hide file tree
Showing 8 changed files with 453 additions and 139 deletions.
142 changes: 113 additions & 29 deletions packages/react-dom-bindings/src/client/validateDOMNesting.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 = '';
Expand All @@ -478,33 +548,45 @@ function validateDOMNesting(
' Add a <tbody>, <thead> or <tfoot> 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;
Expand All @@ -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;
Expand Down
Loading

0 comments on commit 2d3f81b

Please sign in to comment.