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

Fixes children when using dangerouslySetInnerHtml in a selected <option> #13078

Merged
merged 6 commits into from
Jun 21, 2018
Merged

Fixes children when using dangerouslySetInnerHtml in a selected <option> #13078

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7911335
Fixes children when using dangerouslySetInnerHtml in a selected <option>
mridgway Jun 20, 2018
f4455cd
PR fix and new ReactDOMServerIntegrationForms test
mridgway Jun 20, 2018
d752c77
Account for null case
mridgway Jun 21, 2018
9314c57
Combine test cases into single test
mridgway Jun 21, 2018
988f1df
Add tests for failure cases
mridgway Jun 21, 2018
c09d2ab
Fix lint
mridgway Jun 21, 2018
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
29 changes: 29 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMSelect-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,35 @@ describe('ReactDOMSelect', () => {
expect(markup).not.toContain('<option selected="" value="gorilla"');
});

it('should support server-side rendering with dangerouslySetInnerHTML', () => {
const stub = (
<select defaultValue="giraffe">
<option
value="monkey"
dangerouslySetInnerHTML={{
__html: 'A monkey!',
}}
/>
<option
value="giraffe"
dangerouslySetInnerHTML={{
__html: 'A giraffe!',
}}
/>
<option
value="gorilla"
dangerouslySetInnerHTML={{
__html: 'A gorilla!',
}}
/>
</select>
);
const markup = ReactDOMServer.renderToString(stub);
expect(markup).toContain('<option selected="" value="giraffe"');
expect(markup).not.toContain('<option selected="" value="monkey"');
expect(markup).not.toContain('<option selected="" value="gorilla"');
});

it('should support server-side rendering with multiple', () => {
const stub = (
<select multiple={true} value={['giraffe', 'gorilla']} onChange={noop}>
Expand Down
4 changes: 3 additions & 1 deletion packages/react-dom/src/server/ReactPartialRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1173,7 +1173,9 @@ class ReactDOMServerRenderer {
} else if (tag === 'option') {
let selected = null;
const selectValue = this.currentSelectValue;
const optionChildren = flattenOptionChildren(props.children);
const optionChildren = Array.isArray(props.children)
? flattenOptionChildren(props.children)
: props.children;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking for an Array isn't sufficient since React supports using iterables as children. Instead, can we invert the logic so that it checks for undefined?

const optionChildren = props.children === undefined
    ? props.children
    : flattenOptionChildren(props.children)

Alternatively we could just add that check to flattenOptionChildren directly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're totally right. I'm not sure all of the possible cases here. I do know that null is also possible in this case. Would null and undefined checks be sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If children was null then the user must have explicitly rendered with null as a child. In that case we still want to throw because that counts as using children. We just want to handle the case where there are no children at all, so just checking undefined is sufficient.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case we still want to throw because that counts as using children

Do we? On the client we only fire the invariant with == null check so neither null nor undefined counts:

react/src/renderers/dom/shared/ReactDOMComponent.js

Lines 161 to 164 in 571a920

invariant(
props.children == null,
'Can only set one of `children` or `props.dangerouslySetInnerHTML`.',
);

Server logic should be the same, shouldn't it? Or am I missing something?

We should have a test for this too I guess.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we threw for null. You're right, server logic should be the same.

if (selectValue != null) {
let value;
if (props.value != null) {
Expand Down