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 ReactNode as a type for the child of <option/> #15513

Closed
vandercloak opened this issue Apr 26, 2019 · 17 comments
Closed

Allow ReactNode as a type for the child of <option/> #15513

vandercloak opened this issue Apr 26, 2019 · 17 comments
Labels
Resolution: Stale Automatically closed due to inactivity Type: Feature Request

Comments

@vandercloak
Copy link

vandercloak commented Apr 26, 2019

Do you want to request a feature or report a bug?
Feature

What is the current behavior?
Currently, the options element only allows types number and string.

What is the expected behavior?
An option should allow for a ReactNode as a child in addition to a number + string.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
All versions.
All browser types.
To the best of my knowledge, no.

p.s. This is my first feature request here, so let me know if I need to adjust the feature request in any way.

@threepointone
Copy link
Contributor

It would be good if you could summarise that previous thread, with examples of current behaviour, expected behaviour, and such.

@kambleaa007
Copy link

kambleaa007 commented Apr 29, 2019

current behaviour:
inside <p> compo string works,
but,
inside FormattedMessage's <option> not works,
https://codesandbox.io/s/9220lkmk64?fontsize=14

expected behaviour:
There's no bug? should be render in <option> tag too.
https://codesandbox.io/s/9220lkmk64?fontsize=14

I'm afraid same issue,
FormattedMessage in Select's options #286
formatjs/formatjs#286

https://stackoverflow.com/questions/33441524/how-to-use-formattedmessage-inside-an-option-tag-in-react-0-14

@jeremytowne
Copy link

it seems @kambleaa007's example didn't compile, but I fixed it: https://codesandbox.io/s/mw9y04j5j?fontsize=14

@engprodigy
Copy link

it seems @kambleaa007's example didn't compile, but I fixed it: https://codesandbox.io/s/mw9y04j5j?fontsize=14

I think the solution you provided solves the issue. I think this can be closed.

@vandercloak
Copy link
Author

it seems @kambleaa007's example didn't compile, but I fixed it: https://codesandbox.io/s/mw9y04j5j?fontsize=14

I think the solution you provided solves the issue. I think this can be closed.

I am still not satisfied with that approach. The issue (or feature request) is for option to accept ReactNode as a child.

The sandbox shows that a ReactNode returns "[object Object]" instead of rendering the the child as a string.

The fact that option does not behave like the majority of other react elements (could be wrong on this, not an expert but just my limited experience) leads me to think that this feature is still worthwhile and beneficial as it will enhance option to be work as one would expect.

Plus it is just much simpler to drop a component in rather than going through the route that is shown.

@vandercloak vandercloak changed the title Allow JSX.element as a type for the child of <option/> Allow ReactNode as a type for the child of <option/> May 16, 2019
@engprodigy
Copy link

@vandercloak

This has always been an issue. React < 0.14 used to silently accept invalid DOM structure, in your case elements inside elements. The browser would then correct the DOM structure, and cause the virtual DOM managed by React to be out of sync with the real thing. You wouldn't see errors until you tried to re-render existing components instead of just re-mounting them.

react-intl V2.0.0, which will ship with support for React 0.14, allows you to use the Function-As-Child pattern to customize the way your Formatted* components render. See the "Function-As-Child Support" paragraph on this issue.

@lukescott
Copy link

@engprodigy

ReactNode can either be ReactElement | ReactNode[] | string | number | boolean | null. As long as the resulting ReactNode is a text node there shouldn't be an issue. The browser would have nothing to correct.

Just because react-intl provides a workaround doesn't mean the inconsistency of <option> vs every other element in React should be ignored.

@engprodigy
Copy link

engprodigy commented May 23, 2019

If we think about it this way, tag has predictable usage and usually has string (text) as a child, anything more than that will be bogus for the DOM to correctly interpret and it defaults everything to a string.
Check this html codes:
https://www.w3schools.com/code/tryit.asp?filename=G4BYJKZ7S1PR and inspect it in your browser

You should probably not use a select and option element if you want anything more than a string as a child of option. You can combine other custom (your own defined) react elements and react component to achieve what you want to achieve with the default behavior of the combination of select and option react element.

@lukescott
Copy link

lukescott commented May 24, 2019

@engprodigy I'm not sure you understand the request. The request is not to allow <option><div>...</div></option>. It's to allow <option><>foo</></option>, or any React component that results in a string. This has nothing to do with producing invalid HTML.

React components are not always DOM elements. This is a valid React component:

function Hello() {
  return "Hello World"
}

When you use <div><Hello /></div> and view it in the document inspector you see <div>Hello World</div>. Likewise <option><Hello /></option> should result in <option>Hello World</option> not <option>[object Object]</option>. <option><><Hello /></></option> should also result in <option>Hello World</option> as it does with a div.

I think the technical challenge mentioned is if you do <option><div><Hello /></div></option> the browser changes this from <option><div>Hello World</div></option> to <option>Hello World</option> making React out of sync. But with <option><Hello /></option> or <option><><Hello /></></option> being <option>Hello World</option>, there is nothing for the browser to change. At least visually. I'm not certain if React would create multiple Text Nodes, or whether these Text Nodes would get merged also forcing a sync problem.

<option> should support a React component that returns a string OR a fragment of components that return strings. If an HTMLElement is returned it should throw an error.

This is more for consistency sake. We have something along the lines of:

<ul>
  <li>
    <label>
      <input type="radio" value={0} />
      Option 1 (<FormatCurrency value="20" />)
    </label>
  </li>
  <li>
    <label>
      <input type="radio" value={1} />
      Option 2 (<FormatCurrency value="40" />)
    </label>
  </li>
</ul>

And with changing from radios to a dropdown, we would expect to be able to do this:

<select>
  <option value={0}>
    Option 1 (<FormatCurrency value="20" />)
  </option>
  <option value={1}>
    Option 2 (<FormatCurrency value="40" />)
  </option>
</select>

But this fails. The FormatCurrency component returns a string. It pulls in context that it needs to figure out which currency it is currently formatting. As a workaround we've had to do something like this:

<select>
  <option value={0}>
    Option 1 ({formatCurrency(20, this.context.currency)})
  </option>
  <option value={1}>
    Option 2 ({formatCurrency(40, this.context.currency)})
  </option>
</select>

We do not wish to expose context to the component containing the options.

The reason we want this fixed is because it creates a lot of foot-gun problems with our engineers. A few of us (from different teams) have experienced this issue on our own and eventually came across the bug listed at the head of this feature request. It is not obvious that it "does not work as expected". It simply changes the text to [object Object].

@lukescott
Copy link

It's amazing this is not considered a regression:

React 16.4.2 works: https://codesandbox.io/s/gifted-beaver-xjv1j
React 16.8.6 breaks (started with 16.5): https://codesandbox.io/s/angry-carson-8rnd3

Same code in both instances:

import React from "react";
import ReactDOM from "react-dom";

const App = () => {
  return (
    <select>
      <option>
        <Foo />
      </option>
    </select>
  );
};

function Foo() {
  return "foo";
}

ReactDOM.render(<App />, document.getElementById("root"));

@kambleaa007
Copy link

Great Catch

@engprodigy
Copy link

@engprodigy I'm not sure you understand the request. The request is not to allow <option><div>...</div></option>. It's to allow <option><>foo</></option>, or any React component that results in a string. This has nothing to do with producing invalid HTML.

React components are not always DOM elements. This is a valid React component:

function Hello() {
  return "Hello World"
}

When you use <div><Hello /></div> and view it in the document inspector you see <div>Hello World</div>. Likewise <option><Hello /></option> should result in <option>Hello World</option> not <option>[object Object]</option>. <option><><Hello /></></option> should also result in <option>Hello World</option> as it does with a div.

I think the technical challenge mentioned is if you do <option><div><Hello /></div></option> the browser changes this from <option><div>Hello World</div></option> to <option>Hello World</option> making React out of sync. But with <option><Hello /></option> or <option><><Hello /></></option> being <option>Hello World</option>, there is nothing for the browser to change. At least visually. I'm not certain if React would create multiple Text Nodes, or whether these Text Nodes would get merged also forcing a sync problem.

<option> should support a React component that returns a string OR a fragment of components that return strings. If an HTMLElement is returned it should throw an error.

This is more for consistency sake. We have something along the lines of:

<ul>
  <li>
    <label>
      <input type="radio" value={0} />
      Option 1 (<FormatCurrency value="20" />)
    </label>
  </li>
  <li>
    <label>
      <input type="radio" value={1} />
      Option 2 (<FormatCurrency value="40" />)
    </label>
  </li>
</ul>

And with changing from radios to a dropdown, we would expect to be able to do this:

<select>
  <option value={0}>
    Option 1 (<FormatCurrency value="20" />)
  </option>
  <option value={1}>
    Option 2 (<FormatCurrency value="40" />)
  </option>
</select>

But this fails. The FormatCurrency component returns a string. It pulls in context that it needs to figure out which currency it is currently formatting. As a workaround we've had to do something like this:

<select>
  <option value={0}>
    Option 1 ({formatCurrency(20, this.context.currency)})
  </option>
  <option value={1}>
    Option 2 ({formatCurrency(40, this.context.currency)})
  </option>
</select>

We do not wish to expose context to the component containing the options.

The reason we want this fixed is because it creates a lot of foot-gun problems with our engineers. A few of us (from different teams) have experienced this issue on our own and eventually came across the bug listed at the head of this feature request. It is not obvious that it "does not work as expected". It simply changes the text to [object Object].

@lukescott I will agree if all the components and elements inserted in an option tag are expected to return a string only, then it should work.
@gaearon I think this is a valid feature request.
I will try and pick it up and work on it

@stale
Copy link

stale bot commented Jan 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@kambleaa007
Copy link

it seems @kambleaa007's example didn't compile, but I fixed it: https://codesandbox.io/s/mw9y04j5j?fontsize=14

Yeh, Thanks

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@stale
Copy link

stale bot commented Apr 9, 2020

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any additional information, please include with in your comment!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Apr 9, 2020
@stale
Copy link

stale bot commented Apr 17, 2020

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@stale stale bot closed this as completed Apr 17, 2020
@gaearon
Copy link
Collaborator

gaearon commented Jun 10, 2021

This is fixed in 18 Alpha.
https://codesandbox.io/s/thirsty-austin-yzgyg?file=/src/index.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Stale Automatically closed due to inactivity Type: Feature Request
Projects
None yet
Development

No branches or pull requests

7 participants