From 9e9d3e5f00c5e83826495b74ae5862e863e88df8 Mon Sep 17 00:00:00 2001
From: Sebastian Markbage <sema@fb.com>
Date: Tue, 4 May 2021 16:01:20 -0400
Subject: [PATCH] Allow complex objects as children of option only if value is
 provided

---
 .../src/__tests__/ReactDOMOption-test.js      | 115 +++++++++++++++---
 .../ReactDOMServerIntegrationSelect-test.js   |  28 ++---
 .../react-dom/src/client/ReactDOMComponent.js |   8 +-
 .../src/client/ReactDOMHostConfig.js          |   1 -
 .../react-dom/src/client/ReactDOMOption.js    |  76 ++++--------
 .../src/server/ReactDOMServerFormatConfig.js  |  17 ++-
 .../src/server/ReactPartialRenderer.js        |  20 ++-
 7 files changed, 164 insertions(+), 101 deletions(-)

diff --git a/packages/react-dom/src/__tests__/ReactDOMOption-test.js b/packages/react-dom/src/__tests__/ReactDOMOption-test.js
index a81dd32f1f1ba..4271b87386c2e 100644
--- a/packages/react-dom/src/__tests__/ReactDOMOption-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMOption-test.js
@@ -12,12 +12,14 @@
 describe('ReactDOMOption', () => {
   let React;
   let ReactDOM;
+  let ReactDOMServer;
   let ReactTestUtils;
 
   beforeEach(() => {
     jest.resetModules();
     React = require('react');
     ReactDOM = require('react-dom');
+    ReactDOMServer = require('react-dom/server');
     ReactTestUtils = require('react-dom/test-utils');
   });
 
@@ -32,9 +34,9 @@ describe('ReactDOMOption', () => {
     expect(node.innerHTML).toBe('1 foo');
   });
 
-  it('should ignore and warn invalid children types', () => {
+  it('should warn for invalid child tags', () => {
     const el = (
-      <option>
+      <option value="12">
         {1} <div /> {2}
       </option>
     );
@@ -42,10 +44,45 @@ describe('ReactDOMOption', () => {
     expect(() => {
       node = ReactTestUtils.renderIntoDocument(el);
     }).toErrorDev(
-      'Only strings and numbers are supported as <option> children.\n' +
+      'validateDOMNesting(...): <div> cannot appear as a child of <option>.\n' +
+        '    in div (at **)\n' +
         '    in option (at **)',
     );
-    expect(node.innerHTML).toBe('1 [object Object] 2');
+    expect(node.innerHTML).toBe('1 <div></div> 2');
+    ReactTestUtils.renderIntoDocument(el);
+  });
+
+  it('should warn for component child if no value prop is provided', () => {
+    function Foo() {
+      return '2';
+    }
+    const el = (
+      <option>
+        {1} <Foo /> {3}
+      </option>
+    );
+    let node;
+    expect(() => {
+      node = ReactTestUtils.renderIntoDocument(el);
+    }).toErrorDev(
+      'Cannot infer the option value of complex children. ' +
+        'Pass a `value` prop or use a plain string as children to <option>.',
+    );
+    expect(node.innerHTML).toBe('1 2 3');
+    ReactTestUtils.renderIntoDocument(el);
+  });
+
+  it('should not warn for component child if value prop is provided', () => {
+    function Foo() {
+      return '2';
+    }
+    const el = (
+      <option value="123">
+        {1} <Foo /> {3}
+      </option>
+    );
+    const node = ReactTestUtils.renderIntoDocument(el);
+    expect(node.innerHTML).toBe('1 2 3');
     ReactTestUtils.renderIntoDocument(el);
   });
 
@@ -91,7 +128,7 @@ describe('ReactDOMOption', () => {
 
   it('should support element-ish child', () => {
     // This is similar to <fbt>.
-    // It's important that we toString it.
+    // We don't toString it because you must instead provide a value prop.
     const obj = {
       $$typeof: Symbol.for('react.element'),
       type: props => props.content,
@@ -105,37 +142,42 @@ describe('ReactDOMOption', () => {
       },
     };
 
-    let node = ReactTestUtils.renderIntoDocument(<option>{obj}</option>);
+    let node = ReactTestUtils.renderIntoDocument(
+      <option value="a">{obj}</option>,
+    );
     expect(node.innerHTML).toBe('hello');
 
-    node = ReactTestUtils.renderIntoDocument(<option>{[obj]}</option>);
+    node = ReactTestUtils.renderIntoDocument(
+      <option value="b">{[obj]}</option>,
+    );
     expect(node.innerHTML).toBe('hello');
 
-    expect(() => {
-      node = ReactTestUtils.renderIntoDocument(
-        <option>
-          {obj}
-          <span />
-        </option>,
-      );
-    }).toErrorDev(
-      'Only strings and numbers are supported as <option> children.',
+    node = ReactTestUtils.renderIntoDocument(
+      <option value={obj}>{obj}</option>,
     );
-    expect(node.innerHTML).toBe('hello[object Object]');
+    expect(node.innerHTML).toBe('hello');
+    expect(node.value).toBe('hello');
 
     node = ReactTestUtils.renderIntoDocument(
-      <option>
+      <option value={obj}>
         {'1'}
         {obj}
         {2}
       </option>,
     );
     expect(node.innerHTML).toBe('1hello2');
+    expect(node.value).toBe('hello');
   });
 
   it('should be able to use dangerouslySetInnerHTML on option', () => {
     const stub = <option dangerouslySetInnerHTML={{__html: 'foobar'}} />;
-    const node = ReactTestUtils.renderIntoDocument(stub);
+    let node;
+    expect(() => {
+      node = ReactTestUtils.renderIntoDocument(stub);
+    }).toErrorDev(
+      'Pass a `value` prop if you set dangerouslyInnerHTML so React knows which value should be selected.\n' +
+        '    in option (at **)',
+    );
 
     expect(node.innerHTML).toBe('foobar');
   });
@@ -169,4 +211,39 @@ describe('ReactDOMOption', () => {
     ReactDOM.render(<select value="gorilla">{options}</select>, container);
     expect(node.selectedIndex).toEqual(2);
   });
+
+  it('generates a warning and hydration error when an invalid nested tag is used as a child', () => {
+    const ref = React.createRef();
+    const children = (
+      <select readOnly={true} value="bar">
+        <option value="bar">
+          {['Bar', false, 'Foo', <div key="1" ref={ref} />, 'Baz']}
+        </option>
+      </select>
+    );
+
+    const container = document.createElement('div');
+
+    container.innerHTML = ReactDOMServer.renderToString(children);
+
+    expect(container.firstChild.getAttribute('value')).toBe(null);
+    expect(container.firstChild.getAttribute('defaultValue')).toBe(null);
+
+    const option = container.firstChild.firstChild;
+    expect(option.nodeName).toBe('OPTION');
+
+    expect(option.textContent).toBe('BarFooBaz');
+    expect(option.selected).toBe(true);
+
+    expect(() => ReactDOM.hydrate(children, container)).toErrorDev([
+      'Text content did not match. Server: "FooBaz" Client: "Foo"',
+      'validateDOMNesting(...): <div> cannot appear as a child of <option>.',
+    ]);
+
+    expect(option.textContent).toBe('BarFooBaz');
+    expect(option.selected).toBe(true);
+
+    expect(ref.current.nodeName).toBe('DIV');
+    expect(ref.current.parentNode).toBe(option);
+  });
 });
diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationSelect-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationSelect-test.js
index 3871ad975e743..1ad9b0bb64540 100644
--- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationSelect-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationSelect-test.js
@@ -156,13 +156,12 @@ describe('ReactDOMServerIntegrationSelect', () => {
           </option>
           <option
             id="baz"
-            value="baz"
             dangerouslySetInnerHTML={{
-              __html: 'Baz',
+              __html: 'Baz', // This warns because no value prop is passed.
             }}
           />
         </select>,
-        1,
+        2,
       );
       expectSelectValue(e, 'bar');
     },
@@ -228,26 +227,23 @@ describe('ReactDOMServerIntegrationSelect', () => {
       </select>,
     );
     const option = e.options[0];
-    expect(option.childNodes.length).toBe(1);
-    expect(option.childNodes[0].nodeType).toBe(3);
-    expect(option.childNodes[0].nodeValue).toBe('A B');
+    expect(option.textContent).toBe('A B');
+    expect(option.value).toBe('bar');
+    expect(option.selected).toBe(true);
   });
 
   itRenders(
-    'a select option with flattened children and a warning',
+    'a select option with flattened children no value',
     async render => {
       const e = await render(
-        <select readOnly={true} value="bar">
-          <option value="bar">
-            {['Bar', false, 'Foo', <div key="1" />, 'Baz']}
-          </option>
+        <select value="A B" readOnly={true}>
+          <option>A {'B'}</option>
         </select>,
-        1,
       );
-      expect(e.getAttribute('value')).toBe(null);
-      expect(e.getAttribute('defaultValue')).toBe(null);
-      expect(e.firstChild.innerHTML).toBe('BarFoo[object Object]Baz');
-      expect(e.firstChild.selected).toBe(true);
+      const option = e.options[0];
+      expect(option.textContent).toBe('A B');
+      expect(option.value).toBe('A B');
+      expect(option.selected).toBe(true);
     },
   );
 });
diff --git a/packages/react-dom/src/client/ReactDOMComponent.js b/packages/react-dom/src/client/ReactDOMComponent.js
index 095512b75b75c..635c67f5d7700 100644
--- a/packages/react-dom/src/client/ReactDOMComponent.js
+++ b/packages/react-dom/src/client/ReactDOMComponent.js
@@ -29,7 +29,6 @@ import {
   restoreControlledState as ReactDOMInputRestoreControlledState,
 } from './ReactDOMInput';
 import {
-  getHostProps as ReactDOMOptionGetHostProps,
   postMountWrapper as ReactDOMOptionPostMountWrapper,
   validateProps as ReactDOMOptionValidateProps,
 } from './ReactDOMOption';
@@ -535,7 +534,7 @@ export function setInitialProperties(
       break;
     case 'option':
       ReactDOMOptionValidateProps(domElement, rawProps);
-      props = ReactDOMOptionGetHostProps(domElement, rawProps);
+      props = rawProps;
       break;
     case 'select':
       ReactDOMSelectInitWrapperState(domElement, rawProps);
@@ -615,11 +614,6 @@ export function diffProperties(
       nextProps = ReactDOMInputGetHostProps(domElement, nextRawProps);
       updatePayload = [];
       break;
-    case 'option':
-      lastProps = ReactDOMOptionGetHostProps(domElement, lastRawProps);
-      nextProps = ReactDOMOptionGetHostProps(domElement, nextRawProps);
-      updatePayload = [];
-      break;
     case 'select':
       lastProps = ReactDOMSelectGetHostProps(domElement, lastRawProps);
       nextProps = ReactDOMSelectGetHostProps(domElement, nextRawProps);
diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js
index 22702453fd133..908719d679891 100644
--- a/packages/react-dom/src/client/ReactDOMHostConfig.js
+++ b/packages/react-dom/src/client/ReactDOMHostConfig.js
@@ -351,7 +351,6 @@ export function prepareUpdate(
 export function shouldSetTextContent(type: string, props: Props): boolean {
   return (
     type === 'textarea' ||
-    type === 'option' ||
     type === 'noscript' ||
     typeof props.children === 'string' ||
     typeof props.children === 'number' ||
diff --git a/packages/react-dom/src/client/ReactDOMOption.js b/packages/react-dom/src/client/ReactDOMOption.js
index 56a4efe631519..b9103242c35c4 100644
--- a/packages/react-dom/src/client/ReactDOMOption.js
+++ b/packages/react-dom/src/client/ReactDOMOption.js
@@ -12,27 +12,7 @@ import {getToStringValue, toString} from './ToStringValue';
 
 let didWarnSelectedSetOnOption = false;
 let didWarnInvalidChild = false;
-
-function flattenChildren(children) {
-  let content = '';
-
-  // Flatten children. We'll warn if they are invalid
-  // during validateProps() which runs for hydration too.
-  // Note that this would throw on non-element objects.
-  // Elements are stringified (which is normally irrelevant
-  // but matters for <fbt>).
-  Children.forEach(children, function(child) {
-    if (child == null) {
-      return;
-    }
-    content += (child: any);
-    // Note: we don't warn about invalid children here.
-    // Instead, this is done separately below so that
-    // it happens during the hydration code path too.
-  });
-
-  return content;
-}
+let didWarnInvalidInnerHTML = false;
 
 /**
  * Implements an <option> host component that warns when `selected` is set.
@@ -40,28 +20,33 @@ function flattenChildren(children) {
 
 export function validateProps(element: Element, props: Object) {
   if (__DEV__) {
-    // This mirrors the code path above, but runs for hydration too.
-    // Warn about invalid children here so that client and hydration are consistent.
-    // TODO: this seems like it could cause a DEV-only throw for hydration
-    // if children contains a non-element object. We should try to avoid that.
-    if (typeof props.children === 'object' && props.children !== null) {
-      Children.forEach(props.children, function(child) {
-        if (child == null) {
-          return;
-        }
-        if (typeof child === 'string' || typeof child === 'number') {
-          return;
-        }
-        if (typeof (child: any).type !== 'string') {
-          return;
-        }
-        if (!didWarnInvalidChild) {
-          didWarnInvalidChild = true;
+    // If a value is not provided, then the children must be simple.
+    if (props.value == null) {
+      if (typeof props.children === 'object' && props.children !== null) {
+        Children.forEach(props.children, function(child) {
+          if (child == null) {
+            return;
+          }
+          if (typeof child === 'string' || typeof child === 'number') {
+            return;
+          }
+          if (!didWarnInvalidChild) {
+            didWarnInvalidChild = true;
+            console.error(
+              'Cannot infer the option value of complex children. ' +
+                'Pass a `value` prop or use a plain string as children to <option>.',
+            );
+          }
+        });
+      } else if (props.dangerouslySetInnerHTML != null) {
+        if (!didWarnInvalidInnerHTML) {
+          didWarnInvalidInnerHTML = true;
           console.error(
-            'Only strings and numbers are supported as <option> children.',
+            'Pass a `value` prop if you set dangerouslyInnerHTML so React knows ' +
+              'which value should be selected.',
           );
         }
-      });
+      }
     }
 
     // TODO: Remove support for `selected` in <option>.
@@ -81,14 +66,3 @@ export function postMountWrapper(element: Element, props: Object) {
     element.setAttribute('value', toString(getToStringValue(props.value)));
   }
 }
-
-export function getHostProps(element: Element, props: Object) {
-  const hostProps = {children: undefined, ...props};
-  const content = flattenChildren(props.children);
-
-  if (content) {
-    hostProps.children = content;
-  }
-
-  return hostProps;
-}
diff --git a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js
index 941ee252be003..09ac84a673220 100644
--- a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js
+++ b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js
@@ -563,6 +563,7 @@ let didWarnDefaultChecked = false;
 let didWarnDefaultSelectValue = false;
 let didWarnDefaultTextareaValue = false;
 let didWarnInvalidOptionChildren = false;
+let didWarnInvalidOptionInnerHTML = false;
 let didWarnSelectedSetOnOption = false;
 
 function checkSelectProp(props, propName) {
@@ -667,7 +668,8 @@ function flattenOptionChildren(children: mixed): string {
       ) {
         didWarnInvalidOptionChildren = true;
         console.error(
-          'Only strings and numbers are supported as <option> children.',
+          'Cannot infer the option value of complex children. ' +
+            'Pass a `value` prop or use a plain string as children to <option>.',
         );
       }
     }
@@ -736,7 +738,18 @@ function pushStartOption(
     if (value !== null) {
       stringValue = '' + value;
     } else {
-      stringValue = children = flattenOptionChildren(children);
+      if (__DEV__) {
+        if (innerHTML !== null) {
+          if (!didWarnInvalidOptionInnerHTML) {
+            didWarnInvalidOptionInnerHTML = true;
+            console.error(
+              'Pass a `value` prop if you set dangerouslyInnerHTML so React knows ' +
+                'which value should be selected.',
+            );
+          }
+        }
+      }
+      stringValue = flattenOptionChildren(children);
     }
     if (isArray(selectedValue)) {
       // multiple
diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js
index 90fbfd6ad4391..dd568c16363e3 100644
--- a/packages/react-dom/src/server/ReactPartialRenderer.js
+++ b/packages/react-dom/src/server/ReactPartialRenderer.js
@@ -188,6 +188,7 @@ let didWarnDefaultChecked = false;
 let didWarnDefaultSelectValue = false;
 let didWarnDefaultTextareaValue = false;
 let didWarnInvalidOptionChildren = false;
+let didWarnInvalidOptionInnerHTML = false;
 const didWarnAboutNoopUpdateForComponent = {};
 const didWarnAboutBadClass = {};
 const didWarnAboutModulePatternComponent = {};
@@ -334,7 +335,8 @@ function flattenOptionChildren(children: mixed): ?string {
       ) {
         didWarnInvalidOptionChildren = true;
         console.error(
-          'Only strings and numbers are supported as <option> children.',
+          'Cannot infer the option value of complex children. ' +
+            'Pass a `value` prop or use a plain string as children to <option>.',
         );
       }
     }
@@ -1507,13 +1509,23 @@ class ReactDOMServerRenderer {
     } else if (tag === 'option') {
       let selected = null;
       const selectValue = this.currentSelectValue;
-      const optionChildren = flattenOptionChildren(props.children);
       if (selectValue != null) {
         let value;
         if (props.value != null) {
           value = props.value + '';
         } else {
-          value = optionChildren;
+          if (__DEV__) {
+            if (props.dangerouslySetInnerHTML != null) {
+              if (!didWarnInvalidOptionInnerHTML) {
+                didWarnInvalidOptionInnerHTML = true;
+                console.error(
+                  'Pass a `value` prop if you set dangerouslyInnerHTML so React knows ' +
+                    'which value should be selected.',
+                );
+              }
+            }
+          }
+          value = flattenOptionChildren(props.children);
         }
         selected = false;
         if (isArray(selectValue)) {
@@ -1531,12 +1543,10 @@ class ReactDOMServerRenderer {
         props = Object.assign(
           {
             selected: undefined,
-            children: undefined,
           },
           props,
           {
             selected: selected,
-            children: optionChildren,
           },
         );
       }