Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move validation of text nesting into ReactDOMComponent #26594

Merged
merged 3 commits into from
Apr 11, 2023
Merged

Move validation of text nesting into ReactDOMComponent #26594

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5582069
Extract validateTextNesting from validateDOMNesting
sebmarkbage Apr 10, 2023
9f4b6c1
Move validation of text nesting into ReactDOMComponent
sebmarkbage Apr 11, 2023
05d8d8a
Don't validate custom elements unnecessarily
sebmarkbage Apr 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions packages/react-dom-bindings/src/client/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import {
updateTextarea,
restoreControlledTextareaState,
} from './ReactDOMTextarea';
import {validateTextNesting} from './validateDOMNesting';
import {track} from './inputValueTracking';
import setInnerHTML from './setInnerHTML';
import setTextContent from './setTextContent';
Expand Down Expand Up @@ -279,6 +280,9 @@ function setProp(
switch (key) {
case 'children': {
if (typeof value === 'string') {
if (__DEV__) {
validateTextNesting(value, tag);
}
// Avoid setting initial textContent when the text is empty. In IE11 setting
// textContent on a <textarea> will cause the placeholder to not
// show within the <textarea> until it has been focused and blurred again.
Expand All @@ -290,6 +294,9 @@ function setProp(
setTextContent(domElement, value);
}
} else if (typeof value === 'number') {
if (__DEV__) {
validateTextNesting('' + value, tag);
}
const canSetTextContent = !enableHostSingletons || tag !== 'body';
if (canSetTextContent) {
setTextContent(domElement, '' + value);
Expand Down Expand Up @@ -740,8 +747,14 @@ function setPropOnCustomElement(
}
case 'children': {
if (typeof value === 'string') {
if (__DEV__) {
validateTextNesting(value, tag);
sebmarkbage marked this conversation as resolved.
Show resolved Hide resolved
}
setTextContent(domElement, value);
} else if (typeof value === 'number') {
if (__DEV__) {
validateTextNesting('' + value, tag);
}
setTextContent(domElement, '' + value);
}
break;
Expand Down
41 changes: 11 additions & 30 deletions packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,11 @@ import {
} from './ReactDOMComponent';
import {getSelectionInformation, restoreSelection} from './ReactInputSelection';
import setTextContent from './setTextContent';
import {validateDOMNesting, updatedAncestorInfoDev} from './validateDOMNesting';
import {
validateDOMNesting,
validateTextNesting,
updatedAncestorInfoDev,
} from './validateDOMNesting';
import {
isEnabled as ReactBrowserEventEmitterIsEnabled,
setEnabled as ReactBrowserEventEmitterSetEnabled,
Expand Down Expand Up @@ -328,18 +332,7 @@ export function createInstance(
if (__DEV__) {
// TODO: take namespace into account when validating.
const hostContextDev: HostContextDev = (hostContext: any);
validateDOMNesting(type, null, hostContextDev.ancestorInfo);
if (
typeof props.children === 'string' ||
typeof props.children === 'number'
) {
const string = '' + props.children;
const ownAncestorInfo = updatedAncestorInfoDev(
hostContextDev.ancestorInfo,
type,
);
validateDOMNesting(null, string, ownAncestorInfo);
}
validateDOMNesting(type, hostContextDev.ancestorInfo);
namespace = hostContextDev.namespace;
} else {
const hostContextProd: HostContextProd = (hostContext: any);
Expand Down Expand Up @@ -491,21 +484,6 @@ export function prepareUpdate(
// TODO: Figure out how to validateDOMNesting when children turn into a string.
return null;
}
if (__DEV__) {
const hostContextDev = ((hostContext: any): HostContextDev);
if (
typeof newProps.children !== typeof oldProps.children &&
(typeof newProps.children === 'string' ||
typeof newProps.children === 'number')
) {
const string = '' + newProps.children;
const ownAncestorInfo = updatedAncestorInfoDev(
hostContextDev.ancestorInfo,
type,
);
validateDOMNesting(null, string, ownAncestorInfo);
}
}
return diffProperties(domElement, type, oldProps, newProps);
}

Expand All @@ -529,7 +507,10 @@ export function createTextInstance(
): TextInstance {
if (__DEV__) {
const hostContextDev = ((hostContext: any): HostContextDev);
validateDOMNesting(null, text, hostContextDev.ancestorInfo);
const ancestor = hostContextDev.ancestorInfo.current;
if (ancestor != null) {
validateTextNesting(text, ancestor.tag);
}
}
const textNode: TextInstance = getOwnerDocumentFromRootContainer(
rootContainerInstance,
Expand Down Expand Up @@ -1756,7 +1737,7 @@ export function resolveSingletonInstance(
if (__DEV__) {
const hostContextDev = ((hostContext: any): HostContextDev);
if (validateDOMNestingDev) {
validateDOMNesting(type, null, hostContextDev.ancestorInfo);
validateDOMNesting(type, hostContextDev.ancestorInfo);
}
}
const ownerDocument = getOwnerDocumentFromRootContainer(
Expand Down
67 changes: 33 additions & 34 deletions packages/react-dom-bindings/src/client/validateDOMNesting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,29 +434,14 @@ function findInvalidAncestorForTag(
const didWarn: {[string]: boolean} = {};

function validateDOMNesting(
childTag: ?string,
childText: ?string,
childTag: string,
ancestorInfo: AncestorInfoDev,
): void {
if (__DEV__) {
ancestorInfo = ancestorInfo || emptyAncestorInfoDev;
const parentInfo = ancestorInfo.current;
const parentTag = parentInfo && parentInfo.tag;

if (childText != null) {
if (childTag != null) {
console.error(
'validateDOMNesting: when childText is passed, childTag should be null',
);
}
childTag = '#text';
} else if (childTag == null) {
console.error(
'validateDOMNesting: when childText or childTag must be provided',
);
return;
}

const invalidParent = isTagValidWithParent(childTag, parentTag)
? null
: parentInfo;
Expand All @@ -478,21 +463,7 @@ function validateDOMNesting(
}
didWarn[warnKey] = true;

let tagDisplayName = childTag;
let whitespaceInfo = '';
if (childTag === '#text') {
if (childText != null && /\S/.test(childText)) {
tagDisplayName = 'Text nodes';
} else {
tagDisplayName = 'Whitespace text nodes';
whitespaceInfo =
" Make sure you don't have any extra whitespace between tags on " +
'each line of your source code.';
}
} else {
tagDisplayName = '<' + childTag + '>';
}

const tagDisplayName = '<' + childTag + '>';
if (invalidParent) {
let info = '';
if (ancestorTag === 'table' && childTag === 'tr') {
Expand All @@ -501,10 +472,9 @@ function validateDOMNesting(
'the browser.';
}
console.error(
'validateDOMNesting(...): %s cannot appear as a child of <%s>.%s%s',
'validateDOMNesting(...): %s cannot appear as a child of <%s>.%s',
tagDisplayName,
ancestorTag,
whitespaceInfo,
info,
);
} else {
Expand All @@ -518,4 +488,33 @@ function validateDOMNesting(
}
}

export {updatedAncestorInfoDev, validateDOMNesting};
function validateTextNesting(childText: string, parentTag: string): void {
if (__DEV__) {
if (isTagValidWithParent('#text', parentTag)) {
return;
}

// eslint-disable-next-line react-internal/safe-string-coercion
const warnKey = '#text|' + parentTag;
if (didWarn[warnKey]) {
return;
}
didWarn[warnKey] = true;

if (/\S/.test(childText)) {
console.error(
'validateDOMNesting(...): Text nodes cannot appear as a child of <%s>.',
parentTag,
);
} else {
console.error(
'validateDOMNesting(...): Whitespace text nodes cannot appear as a child of <%s>. ' +
"Make sure you don't have any extra whitespace between tags on " +
'each line of your source code.',
parentTag,
);
}
}
}

export {updatedAncestorInfoDev, validateDOMNesting, validateTextNesting};
51 changes: 51 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1855,6 +1855,57 @@ describe('ReactDOMComponent', () => {
]);
});

it('warns nicely for updating table rows to use text', () => {
const container = document.createElement('div');

function Row({children}) {
return <tr>{children}</tr>;
}

function Foo({children}) {
return <table>{children}</table>;
}

// First is fine.
ReactDOM.render(<Foo />, container);

expect(() => ReactDOM.render(<Foo> </Foo>, container)).toErrorDev([
'Warning: validateDOMNesting(...): Whitespace text nodes cannot ' +
"appear as a child of <table>. Make sure you don't have any extra " +
'whitespace between tags on each line of your source code.' +
'\n in table (at **)' +
'\n in Foo (at **)',
]);

ReactDOM.render(
<Foo>
<tbody>
<Row />
</tbody>
</Foo>,
container,
);

expect(() =>
ReactDOM.render(
<Foo>
<tbody>
<Row>text</Row>
</tbody>
</Foo>,
container,
),
).toErrorDev([
'Warning: validateDOMNesting(...): Text nodes cannot appear as a ' +
'child of <tr>.' +
'\n in tr (at **)' +
'\n in Row (at **)' +
'\n in tbody (at **)' +
'\n in table (at **)' +
'\n in Foo (at **)',
]);
});

it('gives useful context in warnings', () => {
function Row() {
return <tr />;
Expand Down