Skip to content

Commit

Permalink
Merge pull request #4160 from preactjs/dom-validation
Browse files Browse the repository at this point in the history
  • Loading branch information
@marvinhagemeister
marvinhagemeister committed Oct 20, 2023
2 parents 19de3d9 + d19f87e commit 8fb2f0d
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 94 deletions.
140 changes: 65 additions & 75 deletions debug/src/debug.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ export function initDebug() {
let oldCatchError = options._catchError;
let oldRoot = options._root;
let oldHook = options._hook;
let oldCommit = options._commit;
const warnedComponents = !isWeakMapSupported
? null
: {
Expand All @@ -75,8 +74,6 @@ export function initDebug() {
lazyPropTypes: new WeakMap()
};
const deprecations = [];
/** @type {import("./internal.d.ts").VNode[]} */
let checkVNodeDom = [];

options._catchError = (error, vnode, oldVNode, errorInfo) => {
let component = vnode && vnode._component;
Expand Down Expand Up @@ -150,9 +147,6 @@ export function initDebug() {

options._diff = vnode => {
let { type } = vnode;
if ((typeof type === 'string' && isTableElement(type)) || type === 'p') {
checkVNodeDom.push(vnode);
}

hooksAllowed = true;

Expand Down Expand Up @@ -319,6 +313,7 @@ export function initDebug() {
};

options.diffed = vnode => {
const { type, _parent: parent } = vnode;
// Check if the user passed plain objects as children. Note that we cannot
// move this check into `options.vnode` because components can receive
// children in any shape they want (e.g.
Expand All @@ -338,6 +333,70 @@ export function initDebug() {
});
}

if (typeof type === 'string' && (isTableElement(type) || type === 'p')) {
// Avoid false positives when Preact only partially rendered the
// HTML tree. Whilst we attempt to include the outer DOM in our
// validation, this wouldn't work on the server for
// `preact-render-to-string`. There we'd otherwise flood the terminal
// with false positives, which we'd like to avoid.
let domParentName = getClosestDomNodeParentName(parent);
if (domParentName !== '') {
if (type === 'table' && isTableElement(domParentName)) {
console.error(
'Improper nesting of table. Your <table> should not have a table-node parent.' +
serializeVNode(vnode) +
`\n\n${getOwnerStack(vnode)}`
);
} else if (
(type === 'thead' || type === 'tfoot' || type === 'tbody') &&
domParentName !== 'table'
) {
console.error(
'Improper nesting of table. Your <thead/tbody/tfoot> should have a <table> parent.' +
serializeVNode(vnode) +
`\n\n${getOwnerStack(vnode)}`
);
} else if (
type === 'tr' &&
domParentName !== 'thead' &&
domParentName !== 'tfoot' &&
domParentName !== 'tbody' &&
domParentName !== 'table'
) {
console.error(
'Improper nesting of table. Your <tr> should have a <thead/tbody/tfoot/table> parent.' +
serializeVNode(vnode) +
`\n\n${getOwnerStack(vnode)}`
);
} else if (type === 'td' && domParentName !== 'tr') {
console.error(
'Improper nesting of table. Your <td> should have a <tr> parent.' +
serializeVNode(vnode) +
`\n\n${getOwnerStack(vnode)}`
);
} else if (type === 'th' && domParentName !== 'tr') {
console.error(
'Improper nesting of table. Your <th> should have a <tr>.' +
serializeVNode(vnode) +
`\n\n${getOwnerStack(vnode)}`
);
}
} else if (type === 'p') {
let illegalDomChildrenTypes = getDomChildren(vnode).filter(childType =>
ILLEGAL_PARAGRAPH_CHILD_ELEMENTS.test(childType)
);
if (illegalDomChildrenTypes.length) {
console.error(
'Improper nesting of paragraph. Your <p> should not have ' +
illegalDomChildrenTypes.join(', ') +
'as child-elements.' +
serializeVNode(vnode) +
`\n\n${getOwnerStack(vnode)}`
);
}
}
}

hooksAllowed = false;

if (oldDiffed) oldDiffed(vnode);
Expand Down Expand Up @@ -388,75 +447,6 @@ export function initDebug() {
}
}
};

options._commit = (root, queue) => {
for (let i = 0; i < checkVNodeDom.length; i++) {
const vnode = checkVNodeDom[i];

// Check if HTML nesting is valid. We need to do it in `options.diffed`
// so that we can optionally traverse outside the vdom root in case
// it's an island embedded in an existing (and valid) HTML tree.
const { type, _parent: parent } = vnode;

let domParentName = getClosestDomNodeParentName(parent);
if (type === 'table' && isTableElement(domParentName)) {
console.error(
'Improper nesting of table. Your <table> should not have a table-node parent.' +
serializeVNode(vnode) +
`\n\n${getOwnerStack(vnode)}`
);
} else if (
(type === 'thead' || type === 'tfoot' || type === 'tbody') &&
domParentName !== 'table'
) {
console.error(
'Improper nesting of table. Your <thead/tbody/tfoot> should have a <table> parent.' +
serializeVNode(vnode) +
`\n\n${getOwnerStack(vnode)}`
);
} else if (
type === 'tr' &&
domParentName !== 'thead' &&
domParentName !== 'tfoot' &&
domParentName !== 'tbody' &&
domParentName !== 'table'
) {
console.error(
'Improper nesting of table. Your <tr> should have a <thead/tbody/tfoot/table> parent.' +
serializeVNode(vnode) +
`\n\n${getOwnerStack(vnode)}`
);
} else if (type === 'td' && domParentName !== 'tr') {
console.error(
'Improper nesting of table. Your <td> should have a <tr> parent.' +
serializeVNode(vnode) +
`\n\n${getOwnerStack(vnode)}`
);
} else if (type === 'th' && domParentName !== 'tr') {
console.error(
'Improper nesting of table. Your <th> should have a <tr>.' +
serializeVNode(vnode) +
`\n\n${getOwnerStack(vnode)}`
);
} else if (type === 'p') {
let illegalDomChildrenTypes = getDomChildren(vnode).filter(childType =>
ILLEGAL_PARAGRAPH_CHILD_ELEMENTS.test(childType)
);
if (illegalDomChildrenTypes.length) {
console.error(
'Improper nesting of paragraph. Your <p> should not have ' +
illegalDomChildrenTypes.join(', ') +
'as child-elements.' +
serializeVNode(vnode) +
`\n\n${getOwnerStack(vnode)}`
);
}
}
}
checkVNodeDom = [];

if (oldCommit) oldCommit(root, queue);
};
}

const setState = Component.prototype.setState;
Expand Down
2 changes: 1 addition & 1 deletion debug/test/browser/component-stack.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ describe('component stack', () => {
render(<Bar />, scratch);

let lines = getStack(errors).split('\n');
expect(lines[0].indexOf('td') > -1).to.equal(true);
expect(lines[0].indexOf('tr') > -1).to.equal(true);
expect(lines[1].indexOf('Thrower') > -1).to.equal(true);
expect(lines[2].indexOf('Bar') > -1).to.equal(true);
});
Expand Down
44 changes: 26 additions & 18 deletions debug/test/browser/debug.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,45 +355,53 @@ describe('debug', () => {
describe('table markup', () => {
it('missing <tbody>/<thead>/<tfoot>/<table>', () => {
const Table = () => (
<tr>
<td>hi</td>
</tr>
<div>
<tr>
<td>hi</td>
</tr>
</div>
);
render(<Table />, scratch);
expect(console.error).to.be.calledOnce;
});

it('missing <table> with <thead>', () => {
const Table = () => (
<thead>
<tr>
<td>hi</td>
</tr>
</thead>
<div>
<thead>
<tr>
<td>hi</td>
</tr>
</thead>
</div>
);
render(<Table />, scratch);
expect(console.error).to.be.calledOnce;
});

it('missing <table> with <tbody>', () => {
const Table = () => (
<tbody>
<tr>
<td>hi</td>
</tr>
</tbody>
<div>
<tbody>
<tr>
<td>hi</td>
</tr>
</tbody>
</div>
);
render(<Table />, scratch);
expect(console.error).to.be.calledOnce;
});

it('missing <table> with <tfoot>', () => {
const Table = () => (
<tfoot>
<tr>
<td>hi</td>
</tr>
</tfoot>
<div>
<tfoot>
<tr>
<td>hi</td>
</tr>
</tfoot>
</div>
);
render(<Table />, scratch);
expect(console.error).to.be.calledOnce;
Expand Down

0 comments on commit 8fb2f0d

Please sign in to comment.