Skip to content

Commit

Permalink
Allow complex objects as children of option only if value is provided (
Browse files Browse the repository at this point in the history
  • Loading branch information
sebmarkbage authored and koto committed Jun 15, 2021
1 parent 44cf35c commit 842bf78
Show file tree
Hide file tree
Showing 7 changed files with 164 additions and 101 deletions.
115 changes: 96 additions & 19 deletions packages/react-dom/src/__tests__/ReactDOMOption-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});

Expand All @@ -32,20 +34,55 @@ 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>
);
let node;
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);
});

Expand Down Expand Up @@ -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,
Expand All @@ -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');
});
Expand Down Expand Up @@ -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);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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');
},
Expand Down Expand Up @@ -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);
},
);
});
8 changes: 1 addition & 7 deletions packages/react-dom/src/client/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import {
restoreControlledState as ReactDOMInputRestoreControlledState,
} from './ReactDOMInput';
import {
getHostProps as ReactDOMOptionGetHostProps,
postMountWrapper as ReactDOMOptionPostMountWrapper,
validateProps as ReactDOMOptionValidateProps,
} from './ReactDOMOption';
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
1 change: 0 additions & 1 deletion packages/react-dom/src/client/ReactDOMHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -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' ||
Expand Down
76 changes: 25 additions & 51 deletions packages/react-dom/src/client/ReactDOMOption.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,56 +12,41 @@ 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.
*/

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>.
Expand All @@ -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;
}
Loading

0 comments on commit 842bf78

Please sign in to comment.