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

[bug][16.5.0] option returns [object Object] instead of string #13586

Closed
aso1datov opened this issue Sep 7, 2018 · 29 comments
Closed

[bug][16.5.0] option returns [object Object] instead of string #13586

aso1datov opened this issue Sep 7, 2018 · 29 comments

Comments

@aso1datov
Copy link

aso1datov commented Sep 7, 2018

What is the current behavior?
<option> returns [object Object] instead string

Demo: https://codesandbox.io/s/ww5mv2w957

What is the expected behavior?
Expected string

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

@jquense
Copy link
Contributor

jquense commented Sep 7, 2018

FormattedMessage doesn't render a string, it renders a span which is not valid HTML for the inside of an option tag. You should use intl.formatMessage() directly or try setting the https://github.com/yahoo/react-intl/wiki/Components#intlprovider textcomponent to React.Fragment to avoid the extra span

@aso1datov
Copy link
Author

aso1datov commented Sep 7, 2018

@jquense
Sorry, forgot add default tag for FormattedMessage
It doesn't work with React.Fragment too.

@bvaughn
Copy link
Contributor

bvaughn commented Sep 7, 2018

For what it's worth, the same Code Sandbox works with React 16.4 but not 16.5.

Maybe related to #13465? cc @gaearon

I don't have context on that change, so I'm not sure if this outcome is expected/okay or not.

@Simek
Copy link
Contributor

Simek commented Sep 7, 2018

If you remove option tag and add tagName property to FormattedMessage demo works correctly.

Fork: https://codesandbox.io/s/6yr4xy5ll3

@jquense
Copy link
Contributor

jquense commented Sep 7, 2018

It doesn't work with React.Fragment too.

we should make this work probably as well.

IDK how to categorize this breakage, it worked solely because of how forgiving browsers can be (which is not unlimited causing the original bug), but the markup is definitely incorrect. It also definitely broke something that "worked" before

@quazzie
Copy link

quazzie commented Sep 7, 2018

Does not work with a context consumer inside a option either.
https://codesandbox.io/s/pk574rm207

@bvaughn
Copy link
Contributor

bvaughn commented Sep 7, 2018

I'm going to tag this as a regression for now, at least until we're sure one way or another.

@Simek
Copy link
Contributor

Simek commented Sep 7, 2018

@quazzie You can fix that by wrapping value in option tag inside context Consumer render function.

Fork: https://codesandbox.io/s/oom712ov79

@quazzie
Copy link

quazzie commented Sep 7, 2018

@Simek thanks but in my real app i use a component that uses context internally so i can't really use that.
<option><MyComponent value="value" /></option>
and MyComponent uses a context.Consumer, processes value a bit and returns a string.

@aso1datov aso1datov changed the title [bug][16.5.0] option returns [object Object] instead string [bug][16.5.0] option returns [object Object] instead of string Sep 7, 2018
@quazzie
Copy link

quazzie commented Sep 7, 2018

@bvaughn yeah it seems to be connected to #13465
It seems you can't use any component as child to option.
Test with simple functional component yields same problem:
https://codesandbox.io/s/7y5km7r7k6

@Simek
Copy link
Contributor

Simek commented Sep 7, 2018

@quazzie Sure thing, it seems like it's a regression.

For now if you want you can implement tagName property and pass option as value to component in your app. If property has been provided you can wrap String inside component otherwise you can just return String.

To be clear I'm just providing quick solutions until React team decided how to solve that issue.

@gaearon
Copy link
Collaborator

gaearon commented Sep 7, 2018

I don't think it was strictly a regression. This is kind of a thorny area. It was never intentionally supported. It accidentally worked on initial mount but then crashed on updates (#13261). Fixing the crash was more important, so we fixed it to be treated as text content (which it should be). Unfortunately this means putting custom components in the middle is not supported. That's consistent with how textarea and similar elements work.

I think it's better to show invalid output and warn about something that breaks on updates, than to let people use it only to discover it crashes in production. But I can see arguments for why this should be supported when the custom component returns a string. Unfortunately I don't know how to fix it in a way that would both solve the update crashes and support text-only content. I think for now it's reasonable to say putting custom components into <option> doesn't really work (and never quite worked correctly), and ask you to manually provide a string to it.

@gaearon
Copy link
Collaborator

gaearon commented Sep 7, 2018

React 15 also warned about this but we accidentally lost the warning in 16 for cases when components return a string.

screen shot 2018-09-07 at 5 20 32 pm

@gaearon
Copy link
Collaborator

gaearon commented Sep 7, 2018

Since we warned about this before (in 15), and it crashes in unexpected ways, I think the new behavior is actually a bugfix — both for the missing warning, and for inconsistent behavior. It's very unfortunate that the warnings didn't get emitted in some cases (to be clear, it did for components returning DOM nodes — but we missed the case for strings). But I think allowing crashes in this case is worse than addressing the problem head on.

The intended workaround if you're using react-intl is indeed this: https://codesandbox.io/s/6yr4xy5ll3

If you use something else, just make what you're passing to <option> as a child is actually a string: https://codesandbox.io/s/oom712ov79

Both of these cases don't crash on updates, and work correctly. Again, very sorry we only discovered this now.

@nicolas-cherel
Copy link

@gaearon a single children that renders a single string wouldn't trigger the crash. I understand that it's easier to validate and enforce <option> children input, but the truth is, anyone that want to use context to setup an <option> has to leak the context into the parent. That, and having a corner case where you can't use react to setup an element children feels extremely weird.

@gaearon
Copy link
Collaborator

gaearon commented Sep 25, 2018

@nicolas-cherel You're welcome to file a proposal for how it should work (as long as it doesn't include crashes).

@gaearon
Copy link
Collaborator

gaearon commented Sep 25, 2018

To be clear — if you figure out how to make it work in all cases, I think we'd be happy supporting using custom components returning strings for this. I couldn't figure it out yet.

Betree added a commit to opencollective/opencollective-frontend that referenced this issue Nov 14, 2018
The fact that we use these labels inside `<select/>` options prevent us
from implementing this as a React component as for now React does not
support having components inside `<option/>` tags, even if the component
returns only strings.

[This message](facebook/react#13586 (comment))
explains why its not supported (though it has been in the past) and why
it may not be in a near future.
Betree added a commit to opencollective/opencollective-frontend that referenced this issue Nov 16, 2018
The fact that we use these labels inside `<select/>` options prevent us
from implementing this as a React component as for now React does not
support having components inside `<option/>` tags, even if the component
returns only strings.

[This message](facebook/react#13586 (comment))
explains why its not supported (though it has been in the past) and why
it may not be in a near future.
@exogen
Copy link

exogen commented Jan 18, 2019

Shouldn't this be kept open (or is there another issue tracking this?) even if nobody has any implementation ideas yet? It's totally reasonable to expect <option><React.Fragment>Foo</React.Fragment></option> to work, considering the developer knows that the output HTML would in fact be valid.

@gaearon
Copy link
Collaborator

gaearon commented Jan 18, 2019

As a feature request, sure. Please file a new issue or search if one exists?

@tobico
Copy link

tobico commented Feb 10, 2019

@gaearon I see how this would be difficult to fix, but I'd argue that it still counts as a bug. Isn't it reasonable for a user of React to expect option elements to support nesting of components the same way that every other React component does? If it says anywhere in the React docs and tutorial that only certain kinds of components can be nested, I must have missed it.

@Jokinen
Copy link

Jokinen commented Feb 27, 2019

React 15 also warned about this but we accidentally lost the warning in 16 for cases when components return a string.

I am sorry to go a bit off track here, but what is the status for this lost warning? Is it something which won't be re-introduced?

We had the exact case where we were using a translation component returning a string. The end result is that [object Object] gets rendered which is quite unexpected in the context of React.

@vandercloak
Copy link

Just opened a feature request here #15513

@lukescott
Copy link

@gaearon Maybe I'm missing something, but 16.4 did not warn about this code:

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"));

This code worked just fine in React 16.4, but later broke in 16.5. In some of the code examples I've seen from react-intl package, it would render as <option><span>message</span></option> which would cause the warning. But in the example above no such warning occurs because the component returns a string.

16.4.2: https://codesandbox.io/s/gifted-beaver-xjv1j
16.8.6: https://codesandbox.io/s/angry-carson-8rnd3

Could we perhaps reevaluate this as a regression that needs to be fixed rather than an enhancement?

I would expect that <option><span>message</span></option> should be an error while <option>foo</option> (from the example above) should not.

@Slatery
Copy link

Slatery commented Jun 19, 2019

If anyone still needs an answer to the original question, as mentioned in earlier comments and in the documentation here, you cannot use a formatted message in the format of

<select>
    <option value="foo">
        <FormattedMessage id="bar"/>
    </option>
</select>

instead I've found formating it as such does work:

<select>
    <FormattedMessage id="bar">
        {txt => <option value="foo">{txt}</option>}
    </FormattedMessage>
</select>

where txt will return the id's default string for whichever Message Descriptor file is selected

@danielo515
Copy link

So any valid workaround that doesn't involves changing your entire component tree? Many people is using context inside options to provide localisation etc.

@hrupesh
Copy link

hrupesh commented Aug 17, 2020

Is there any solution to this till date?

@wojtekmaj
Copy link

@hrupesh There's still no solution that isn't hacky. What I did and what my personal recommendation would be is to use a replacement over native select element, carefully crafted to keep accessibility levels high, like Reach UI Listbox.

@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

@gaearon
Copy link
Collaborator

gaearon commented Mar 30, 2022

React 18 is released with the fix.
You need to pass value to <option> for this.

https://codesandbox.io/s/epic-sea-r219ps?file=/src/index.js

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

No branches or pull requests