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

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
Collaborator

gaearon commented Jun 20, 2018

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

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:

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.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

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

@mridgway
Copy link
Contributor Author

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.

@gaearon
Copy link
Collaborator

gaearon commented Jun 21, 2018

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

@gaearon
Copy link
Collaborator

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.

@mridgway
Copy link
Contributor Author

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?

@gaearon
Copy link
Collaborator

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.)

@gaearon
Copy link
Collaborator

gaearon commented Jun 21, 2018

Looks like yarn linc failed?

@gaearon gaearon merged commit da5c87b into facebook:master Jun 21, 2018
@gaearon
Copy link
Collaborator

gaearon commented Jun 21, 2018

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants