Move validation of text nesting into ReactDOMComponent (#26594)

Extract validateTextNesting from validateDOMNesting. We only need the parent tag when validating text nodes. Then validate it in setProp.
facebook · Apr 11, 2023 · ac43bf6 · ac43bf6
1 parent ca41adb
commit ac43bf6
Show file tree

Hide file tree

Showing 4 changed files with 102 additions and 64 deletions.
diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js
@@ -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';
@@ -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.
@@ -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);

diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
@@ -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,
@@ -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);
@@ -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);
 }
 
@@ -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,
@@ -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(

diff --git a/packages/react-dom-bindings/src/client/validateDOMNesting.js b/packages/react-dom-bindings/src/client/validateDOMNesting.js
@@ -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;
@@ -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') {
@@ -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 {
@@ -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};
diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js
@@ -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 />;