React: value|defaultValue={Symbol|Function} should be ignored, not stringified

[gaearon]

Regression in master from https://github.com/facebook/react/pull/11534.
Found it thanks to the attribute fixture snapshots.

[nhunzaker]

Just for clarity: This is the correct behavior for Symbols:

With my change, value and defaultValue never pass through the property sanitation layer. React tries to stringify symbols, which throws an error, and functions get turned into "function () {}".

[gaearon]

Now that I looked at this again, the update codepath was already broken. This just made the initial code path match it. So this is actually an improvement. We should still get https://github.com/facebook/react/pull/11741 to completion to have consistent sane behavior, but IMO this is not as urgent as I thought at first.

[kanlanc]

Is it resolved or can I start working on it?

Sorry new to open source

[gaearon]

I think the issue still exists, it’s just more consistent now

[kanlanc]

just for reference how do you call dibs on a bug

[gaearon]

You got it

[t4deu]

Ping @highskillzz, do you still on this issue?

If not, I will take it, ok @gaearon :wink:

[kanlanc]

Sure

[antonfrolovsky]

@gaearon i think this issue completed, yep?

[raunofreiberg]

What's the status on this one? It doesn't seem like this has been resolved since there are no warnings on assigning symbols or functions to the defaultValue prop. Even found a few TODO-s in the ReactDOMInput test.

[gaearon]

Feel free to grab it. I don't know if it's fixed or not but it's worth checking.

[krrishdholakia]

@gaearon is this still open? if so, how do i claim it?

[philipp-spiess]

@krrishdholakia Yes this is still up for grabs if it is not already fixed! Feel free to start looking into it and reach out if you have any issues along the way. 😊

[raunofreiberg]

I checked earlier and I might be wrong but I'm quite sure this was fixed earlier by @nhunzaker

// Same goes for the `value` prop
return (
      <div>
        <input defaultValue={Symbol("test")} />
        <input defaultValue={() => {}} />
      </div>
);

produces

Which indicates that both symbols and functions are not stringified? I've still went ahead and added warnings for the defaultValue prop in #13360. Maybe useful.

[raunofreiberg]

Although, textarea seems to behave differently - not sure if intentionally or not.

function App() {
  return (
    <div>
      <textarea defaultValue={() => {}} />
      <textarea value={() => {}} />
      <textarea value={Symbol("test")} />
    </div>
  );
}

Both functions get stringified and appended as text into the textarea itself. The symbol itself crashes the application with a TypeError: Cannot convert a Symbol value to a string

For reproducing: https://codesandbox.io/s/pj40nrp5px

[philipp-spiess]

@raunofreiberg Thanks for looking into that! I agree that textarea should behave the same 👍

[raunofreiberg]

This looks perfect now. Thank you very much 🙂 If you want a follow-up task, you could work on the same fix for elements.

@philipp-spiess Do you mean working on the select tag 🙂? If so, I'd be happy to investigate further on that as well.

[philipp-spiess]

Whoops. Yeah the select tag 😊 I forgot to escape the HTML.

Perfect - thank you!

[nhunzaker]

@raunofreiberg any remaining work that you know of here?

[raunofreiberg]

Input, textarea and select are now covered. I can't think of any other form elements where these attributes might be an issue. Should be all good.

[nhunzaker]

Right on. Thanks for all of your contributions!

[gaearon]

Can somebody please update the attribute table?

https://github.com/facebook/react/tree/master/fixtures/attribute-behavior

Run the fixture, save the Markdown file and put it in the same folder.

[nhunzaker]

Ack. I'll send out a new snapshot shortly.

[nhunzaker]

PR sent out. Looks like SSR still needs to be covered for textareas and options:

[code4cake]

Hi guys is this issue still open or is someone already working on it?

[philipp-spiess]

@dantesolis: @nhunzaker is working on it, check out: https://github.com/facebook/react/pull/13394 We're also probably gonna revisit the behavior here (https://github.com/facebook/react/issues/13508). I'm going to remove the "good first issue" label for now to avoid future confusion.

[code4cake]

@philipp-spiess thanks, are there any other "good first issues" that I could pick up? thanks

[philipp-spiess]

@dantesolis You can look for good first issue (taken) and see if there was recent activity. If not, you can usually work on them. In our contribution guides we mention a 2 week period before others can start working on the issue as well.

I also think you can take a look at https://github.com/facebook/react/issues/11299. As far as I know there is still at least one test that uses private API and is not tackled by a community member (ReactErrorUtils-test.internal.js).

What's also great is if you can improve our test or type coverage. You'd need to research how to find uncovered lines since I don't have a solution for that right now but Jest comes with a coverage tool which might be helpful.

Let me know if you're stuck with any of the above steps. 🙂

[code4cake]

@philipp-spiess then I would be giving issue-12548 a try, since I think someone already asked for #11299 before me, but I'll also keep an eye open for that one. Thanks.

About adding coverage, yes, jest comes with coverage. I'll check that one as well. ;)

[kambleaa007]

is it solved ?

[caelinsutch]

What ever happened to this?