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

Allow complex objects as children of <option> only if value="..." is provided #21431

Merged
merged 1 commit into from
May 5, 2021

Conversation

sebmarkbage
Copy link
Collaborator

This one has bothered me for a while and as part of rewriting SSR now seems like a good time.

For legacy reasons, we previously used React.Children to flatten the children of <option>. The reason for this is becase we need to know what the value will be so that we can see if it matches what the selected value passed to <select> was to know if this is the selected one. Also, option only accepted text nodes as children anyway (now this is not strictly true because template tags are allowed).

However, at some point we stopped relying on flattening on the client because we just rely on the value that the browser provides after flattening.

React.Children is not really ever best practice because it doesn't allow for certain abstractions and so it lags in capabilities. Ideally it would never be used and it can be dead-code eliminated, but that would never be the case if React itself has a dependency on it. So the goal here is to remove that dependency and to simplify things by just using the regular children path.

However, as I noted in #21373, it's fine that we don't know what the implicit value is if we know the explicit value. In fact, almost all usages of <option> provides an explicit value="..." attribute anyway. If you do that, then why shouldn't you be able to use all the advanced abstraction features in the child position? Such as function components that return strings but also allowing suspending in those as well as the child position of options in Flight.

This PR enables you to pass any children to <option>.

We still rely on flattening on the server if no value is provided. Therefore I always warn if a "value" is not provided and complex children are provided. I also added warning if dangerouslySetInnerHTML is used without a "value" attribute.

Additionally, it's invalid to pass a <div> as a child of <option> because it won't parse as HTML. That automatically gets covered by the DOM nesting warning in this approach. This new approach adds comment nodes between text nodes. That seems to work fine in Chrome but not sure if there's a browser that has issues with those.

Additionally, it might not be valid to pass <fbt> as a child of option because we might have previously relied on it being toString:ed which triggers the string mode. Now it's being rendered as an element which might let it be rendered as a span. That might lead to DOM nesting warnings. We need to find a way to fix that in fbt because this is not the place to fix that. Additionally for the purpose of comparing the value of the option, fbt shouldn't be used. It needs to pass a separate value prop.

@sebmarkbage sebmarkbage requested a review from gaearon May 5, 2021 02:13
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels May 5, 2021
@sizebot
Copy link

sizebot commented May 5, 2021

Comparing: 014edf1...9e9d3e5

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 122.76 kB 122.53 kB = 39.42 kB 39.34 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 129.29 kB 129.06 kB = 41.50 kB 41.43 kB
facebook-www/ReactDOM-prod.classic.js = 407.18 kB 406.59 kB = 75.34 kB 75.22 kB
facebook-www/ReactDOM-prod.modern.js = 395.01 kB 394.42 kB = 73.41 kB 73.28 kB
facebook-www/ReactDOMForked-prod.classic.js = 407.18 kB 406.59 kB = 75.34 kB 75.22 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactDOMServer-dev.classic.js +0.29% 150.15 kB 150.57 kB +0.28% 38.52 kB 38.63 kB
oss-stable/react-dom/cjs/react-dom-server.browser.development.js +0.25% 138.89 kB 139.24 kB +0.25% 37.00 kB 37.10 kB
oss-stable/react-dom/cjs/react-dom-server.node.development.js +0.25% 140.19 kB 140.54 kB +0.25% 37.26 kB 37.36 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.development.js +0.24% 140.73 kB 141.07 kB +0.27% 37.28 kB 37.38 kB
oss-stable/react-dom/umd/react-dom-server.browser.development.js +0.24% 146.40 kB 146.76 kB +0.27% 37.47 kB 37.57 kB
oss-experimental/react-dom/cjs/react-dom-server.node.development.js +0.24% 142.03 kB 142.37 kB +0.27% 37.54 kB 37.64 kB
oss-experimental/react-dom/umd/react-dom-server.browser.development.js +0.24% 148.35 kB 148.71 kB +0.26% 37.73 kB 37.83 kB
facebook-www/ReactDOMServer-dev.modern.js +0.23% 203.07 kB 203.53 kB +0.19% 47.69 kB 47.79 kB

Generated by 🚫 dangerJS against 9e9d3e5

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.

this is nice!

@sebmarkbage sebmarkbage merged commit 8ea1130 into facebook:master May 5, 2021
@gaearon gaearon mentioned this pull request Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants