react
react copied to clipboard
facebook
value|defaultValue={Symbol|Function} should be ignored, not stringified
Regression in master from https://github.com/facebook/react/pull/11534. Found it thanks to the attribute fixture snapshots.
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 () {}".
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.
Ping @highskillzz, do you still on this issue?
If not, I will take it, ok @gaearon :wink:
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.
Feel free to grab it. I don't know if it's fixed or not but it's worth checking.
@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. 😊
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.
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
@raunofreiberg Thanks for looking into that! I agree that textarea should behave the same 👍
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.
Whoops. Yeah the select tag 😊 I forgot to escape the HTML.
Perfect - thank you!
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.
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.
PR sent out. Looks like SSR still needs to be covered for textareas and options:
@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.
@philipp-spiess thanks, are there any other "good first issues" that I could pick up? thanks
@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. 🙂
@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. ;)
Can i look at it? And what is guidelines for contributing? I am new to open source.
I'm going to work on this issue. If you are already working on it, please let me know!
function App() { return (
Hi,
@bvaughn may I now take this? I have examined the code and I believe I've come up with a potential solution
Hey! I've set up a PR to address the issue (SSR side for option elements) https://github.com/facebook/react/pull/22841, @bvaughn or anyone else in the React team, let me know your thoughts!
@philipp-spiess @bvaughn Hi guys, what's the latest status of this PR? Is it still open? I was trying out for both
hey @philipp-spiess is this issue still unresolved , I am new to open source and I would like to give it a try
please let me know some good first issues that I can work on as I am new to open source
Hello, I am Nelson, I am new here and I want to work on the first issue but I need someone to guide me.
Couldn't understand. Is this one still an issue for fixed before? Confused 🙄
Hey is this issued still open because i am interested to resolve it...?
