-
Notifications
You must be signed in to change notification settings - Fork 46.7k
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.
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
Stringify <option> children #13465
Stringify <option> children #13465
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ describe('ReactDOMOption', () => { | |
let ReactTestUtils; | ||
|
||
beforeEach(() => { | ||
jest.resetModules(); | ||
React = require('react'); | ||
ReactDOM = require('react-dom'); | ||
ReactTestUtils = require('react-dom/test-utils'); | ||
|
@@ -41,9 +42,10 @@ describe('ReactDOMOption', () => { | |
expect(() => { | ||
node = ReactTestUtils.renderIntoDocument(el); | ||
}).toWarnDev( | ||
'<div> cannot appear as a child of <option>.\n' + ' in option (at **)', | ||
'Only strings and numbers are supported as <option> children.\n' + | ||
' in option (at **)', | ||
); | ||
expect(node.innerHTML).toBe('1 2'); | ||
expect(node.innerHTML).toBe('1 [object Object] 2'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the real change here. All cases where behavior changed were warnings. |
||
ReactTestUtils.renderIntoDocument(el); | ||
}); | ||
|
||
|
@@ -61,6 +63,76 @@ describe('ReactDOMOption', () => { | |
expect(node.innerHTML).toBe('1 2'); | ||
}); | ||
|
||
it('should throw on object children', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just documenting existing behavior. |
||
expect(() => { | ||
ReactTestUtils.renderIntoDocument(<option>{{}}</option>); | ||
}).toThrow('Objects are not valid as a React child'); | ||
expect(() => { | ||
ReactTestUtils.renderIntoDocument(<option>{[{}]}</option>); | ||
}).toThrow('Objects are not valid as a React child'); | ||
expect(() => { | ||
ReactTestUtils.renderIntoDocument( | ||
<option> | ||
{{}} | ||
<span /> | ||
</option>, | ||
); | ||
}).toThrow('Objects are not valid as a React child'); | ||
expect(() => { | ||
ReactTestUtils.renderIntoDocument( | ||
<option> | ||
{'1'} | ||
{{}} | ||
{2} | ||
</option>, | ||
); | ||
}).toThrow('Objects are not valid as a React child'); | ||
}); | ||
|
||
it('should support element-ish child', () => { | ||
// This is similar to <fbt>. | ||
// It's important that we toString it. | ||
let obj = { | ||
$$typeof: Symbol.for('react.element'), | ||
type: props => props.content, | ||
ref: null, | ||
key: null, | ||
props: { | ||
content: 'hello', | ||
}, | ||
toString() { | ||
return this.props.content; | ||
}, | ||
}; | ||
|
||
let node = ReactTestUtils.renderIntoDocument(<option>{obj}</option>); | ||
expect(node.innerHTML).toBe('hello'); | ||
|
||
node = ReactTestUtils.renderIntoDocument(<option>{[obj]}</option>); | ||
expect(node.innerHTML).toBe('hello'); | ||
|
||
expect(() => { | ||
node = ReactTestUtils.renderIntoDocument( | ||
<option> | ||
{obj} | ||
<span /> | ||
</option>, | ||
); | ||
}).toWarnDev( | ||
'Only strings and numbers are supported as <option> children.', | ||
); | ||
expect(node.innerHTML).toBe('hello[object Object]'); | ||
|
||
node = ReactTestUtils.renderIntoDocument( | ||
<option> | ||
{'1'} | ||
{obj} | ||
{2} | ||
</option>, | ||
); | ||
expect(node.innerHTML).toBe('1hello2'); | ||
}); | ||
|
||
it('should be able to use dangerouslySetInnerHTML on option', () => { | ||
let stub = <option dangerouslySetInnerHTML={{__html: 'foobar'}} />; | ||
const node = ReactTestUtils.renderIntoDocument(stub); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -439,7 +439,7 @@ describe('ReactDOMServerIntegration', () => { | |
); | ||
expect(e.getAttribute('value')).toBe(null); | ||
expect(e.getAttribute('defaultValue')).toBe(null); | ||
expect(e.firstChild.innerHTML).toBe('BarFooBaz'); | ||
expect(e.firstChild.innerHTML).toBe('BarFoo[object Object]Baz'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Already a warning. |
||
expect(e.firstChild.selected).toBe(true); | ||
}, | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,23 +9,24 @@ | |
|
||
import React from 'react'; | ||
import warning from 'shared/warning'; | ||
import {validateDOMNesting, updatedAncestorInfo} from './validateDOMNesting'; | ||
import {getToStringValue, toString} from './ToStringValue'; | ||
|
||
let didWarnSelectedSetOnOption = false; | ||
let didWarnInvalidChild = false; | ||
|
||
function flattenChildren(children) { | ||
let content = ''; | ||
|
||
// Flatten children and warn if they aren't strings or numbers; | ||
// invalid types are ignored. | ||
// 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>). | ||
React.Children.forEach(children, function(child) { | ||
if (child == null) { | ||
return; | ||
} | ||
if (typeof child === 'string' || typeof child === 'number') { | ||
content += child; | ||
} | ||
content += child; | ||
// Note: we don't warn about invalid children here. | ||
// Instead, this is done separately below so that | ||
// it happens during the hydration codepath too. | ||
|
@@ -40,7 +41,10 @@ function flattenChildren(children) { | |
|
||
export function validateProps(element: Element, props: Object) { | ||
if (__DEV__) { | ||
// Warn about invalid children, mirroring the logic above. | ||
// This mirrors the codepath 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) { | ||
React.Children.forEach(props.children, function(child) { | ||
if (child == null) { | ||
|
@@ -49,13 +53,16 @@ export function validateProps(element: Element, props: Object) { | |
if (typeof child === 'string' || typeof child === 'number') { | ||
return; | ||
} | ||
// This is not real ancestor info but it's close enough | ||
// to produce a useful warning for invalid children. | ||
// We don't have access to the real one because the <option> | ||
// fiber has already been popped, and threading it through | ||
// is needlessly annoying. | ||
const ancestorInfo = updatedAncestorInfo(null, 'option'); | ||
validateDOMNesting(child.type, null, ancestorInfo); | ||
if (typeof child.type !== 'string') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will avoid warnings for fbt. |
||
return; | ||
} | ||
if (!didWarnInvalidChild) { | ||
didWarnInvalidChild = true; | ||
warning( | ||
false, | ||
'Only strings and numbers are supported as <option> children.', | ||
); | ||
} | ||
}); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just reused the same warning message we have in SSR. Seems to make more sense than the DOM nesting warning I tried to use before.