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.

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

Conversation

@mridgway
Copy link
Contributor

@mridgway mridgway commented Jun 20, 2018

This fixes an inadvertent cast of undefined children to an empty string when creating an option tag that will be selected:

  <select defaultValue="test">
    <option value='test' dangerouslySetInnerHTML={{ __html: '&rlm; test'}} />
  </select>

This causes an invariant error because both children and dangerouslySetInnerHTML are set.

What is the current behavior?

const React = require('react');
const ReactDOM = require('react-dom/server');

ReactDOM.renderToString(
  <select defaultValue="test">
    <option value='test' dangerouslySetInnerHTML={{ __html: '&rlm; test'}} />
  </select>
);
/Volumes/Projects/shakti/node_modules/fbjs/lib/invariant.js:49
    throw error;
    ^

Invariant Violation: Can only set one of `children` or `props.dangerouslySetInnerHTML`.
    at invariant (/Volumes/Projects/shakti/node_modules/fbjs/lib/invariant.js:42:15)
    at assertValidProps (/Volumes/Projects/shakti/node_modules/react-dom/cjs/react-dom-server.node.development.js:693:33)
    at ReactDOMServerRenderer.renderDOM (/Volumes/Projects/shakti/node_modules/react-dom/cjs/react-dom-server.node.development.js:2674:5)
    at ReactDOMServerRenderer.render (/Volumes/Projects/shakti/node_modules/react-dom/cjs/react-dom-server.node.development.js:2418:21)
    at ReactDOMServerRenderer.read (/Volumes/Projects/shakti/node_modules/react-dom/cjs/react-dom-server.node.development.js:2357:19)
    at Object.renderToString (/Volumes/Projects/shakti/node_modules/react-dom/cjs/react-dom-server.node.development.js:2731:25)
    at Object.<anonymous> (/Volumes/Projects/shakti/test.js:4:22)
    at Module._compile (module.js:635:30)
    at Object.Module._extensions..js (module.js:646:10)
    at Module.load (module.js:554:32)

What is the expected behavior?

<select data-reactroot=""><option selected="" value="test">&rlm; test</option></select>

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

16.4.1 using node 8. This worked in 15.x.

This fixes an inadvertent cast of undefined children to an empty string when creating an option tag that will be selected:

```
  <select defaultValue="test">
    <option value='test' dangerouslySetInnerHTML={{ __html: '&rlm; test'}} />
  </select>
```

This causes an invariant error because both children and dangerouslySetInnerHTML are set.
@gaearon
Copy link
Member

@gaearon gaearon commented Jun 20, 2018

Do you mind adding a test to one of ReactDOMServerIntegration* suites?

Loading

const optionChildren = flattenOptionChildren(props.children);
const optionChildren = Array.isArray(props.children)
? flattenOptionChildren(props.children)
: props.children;
Copy link
Contributor

@aweary aweary Jun 20, 2018

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

Loading

Copy link
Contributor Author

@mridgway mridgway Jun 20, 2018

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?

Loading

Copy link
Contributor

@aweary aweary Jun 20, 2018

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.

Loading

Copy link
Member

@gaearon gaearon Jun 20, 2018

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:

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.

Loading

Copy link
Contributor

@aweary aweary Jun 20, 2018

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.

Loading

Copy link
Member

@gaearon gaearon left a comment

Let's make sure we don't throw for null, and modify the integration test to include this scenario

Loading

@mridgway
Copy link
Contributor Author

@mridgway mridgway commented Jun 21, 2018

Updated to check for null. Not sure whether you prefer what I did with iterating over [null, undefined] for the tests or copy/pasting. Let me know if you want me to update that.

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Jun 21, 2018

I'd prefer copy pasting since there's already too much indirection in that file.

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Jun 21, 2018

Also, can you make a single test with multiple options, each option having a different falsy value (e.g. null, undefined, 0, '')? Then the coverage is more exhaustive and we don't need many tests.

Loading

@mridgway
Copy link
Contributor Author

@mridgway mridgway commented Jun 21, 2018

Yes, for some reason I thought that the option had to be selected, but if any of them are selected they are all run through this code path. So I will combine the tests into one.

We would expect 0 and '' to fail in this case. Do you want a separate test for the failure scenario?

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Jun 21, 2018

Yeah, a separate test for error consistency would be good. (I think we have a few tests in that suite that show how to assert failures.)

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Jun 21, 2018

Looks like yarn linc failed?

Loading

@gaearon gaearon merged commit da5c87b into facebook:master Jun 21, 2018
1 check was pending
Loading
@gaearon
Copy link
Member

@gaearon gaearon commented Jun 21, 2018

Thank you!

Loading

@mridgway mridgway deleted the reactServerSelectFix branch Jun 22, 2018
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
…on> (facebook#13078)

* Fixes children when using dangerouslySetInnerHtml in a selected <option>

This fixes an inadvertent cast of undefined children to an empty string when creating an option tag that will be selected:

```
  <select defaultValue="test">
    <option value='test' dangerouslySetInnerHTML={{ __html: '&rlm; test'}} />
  </select>
```

This causes an invariant error because both children and dangerouslySetInnerHTML are set.

* PR fix and new ReactDOMServerIntegrationForms test

* Account for null case

* Combine test cases into single test

* Add tests for failure cases

* Fix lint
@gaearon gaearon mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants