The Wayback Machine - https://web.archive.org/web/20220427190002/https://github.com/facebook/react/issues/11734
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

value|defaultValue={Symbol|Function} should be ignored, not stringified #11734

Open
gaearon opened this issue Dec 1, 2017 · 48 comments
Open

value|defaultValue={Symbol|Function} should be ignored, not stringified #11734

gaearon opened this issue Dec 1, 2017 · 48 comments

Comments

@gaearon
Copy link
Member

@gaearon gaearon commented Dec 1, 2017

Regression in master from #11534.
Found it thanks to the attribute fixture snapshots.

@nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Dec 1, 2017

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

screen shot 2017-11-30 at 8 22 43 pm

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
Copy link
Member Author

@gaearon gaearon commented Dec 1, 2017

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 #11741 to completion to have consistent sane behavior, but IMO this is not as urgent as I thought at first.

gaearon added a commit that referenced this issue Dec 1, 2017
- the capture attribute changed in #11424
- changes to value/defaultValue handling of functions/Symbols are from #11534, but as per #11734 (comment) this is actually not a new problem so we're okay with it
@kanlanc
Copy link

@kanlanc kanlanc commented Jan 6, 2018

Is it resolved or can I start working on it?

Sorry new to open source

@gaearon
Copy link
Member Author

@gaearon gaearon commented Jan 6, 2018

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

@kanlanc
Copy link

@kanlanc kanlanc commented Jan 6, 2018

just for reference how do you call dibs on a bug

@gaearon
Copy link
Member Author

@gaearon gaearon commented Jan 6, 2018

You got it

@t4deu
Copy link

@t4deu t4deu commented Feb 1, 2018

Ping @highskillzz, do you still on this issue?

If not, I will take it, ok @gaearon 😉

@kanlanc
Copy link

@kanlanc kanlanc commented Feb 2, 2018

Sure

@raunofreiberg
Copy link
Contributor

@raunofreiberg raunofreiberg commented Mar 16, 2018

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
Copy link
Member Author

@gaearon gaearon commented Aug 9, 2018

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

@krrishdholakia
Copy link

@krrishdholakia krrishdholakia commented Aug 10, 2018

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

@philipp-spiess
Copy link
Contributor

@philipp-spiess philipp-spiess commented Aug 10, 2018

@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
Copy link
Contributor

@raunofreiberg raunofreiberg commented Aug 10, 2018

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

image

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
Copy link
Contributor

@raunofreiberg raunofreiberg commented Aug 10, 2018

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
Copy link
Contributor

@philipp-spiess philipp-spiess commented Aug 10, 2018

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

@raunofreiberg
Copy link
Contributor

@raunofreiberg raunofreiberg commented Aug 14, 2018

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.

@code4cake
Copy link

@code4cake code4cake commented Sep 7, 2018

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

@philipp-spiess
Copy link
Contributor

@philipp-spiess philipp-spiess commented Sep 7, 2018

@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 #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
Copy link

@code4cake code4cake commented Sep 10, 2018

@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
Copy link

@kambleaa007 kambleaa007 commented Aug 7, 2019

is it solved ?

@necolas necolas added the React Core Team label Jan 8, 2020
@caelinsutch
Copy link

@caelinsutch caelinsutch commented May 21, 2020

What ever happened to this?

@ktfth
Copy link

@ktfth ktfth commented Mar 30, 2021

I can work on that task? What is needed to finish that task?

@axaysushir
Copy link

@axaysushir axaysushir commented May 1, 2021

Can i look at it? And what is guidelines for contributing? I am new to open source.

@sebuelias
Copy link

@sebuelias sebuelias commented May 16, 2021

Can I take this issue up?

@reez12g
Copy link

@reez12g reez12g commented Jul 21, 2021

I'm going to work on this issue.
If you are already working on it, please let me know!

@Madanaa
Copy link

@Madanaa Madanaa commented Aug 19, 2021

@bvaughn hi I am new here but I want to solve it.

@Pankajadhana97-bit
Copy link

@Pankajadhana97-bit Pankajadhana97-bit commented Aug 20, 2021

function App() {
return (


<textarea defaultValue={() => {}} />
<textarea value={() => {}} />
<textarea value={Symbol("test")} />

);
}

@Mukulbaid63
Copy link

@Mukulbaid63 Mukulbaid63 commented Oct 26, 2021

@bvaughn Can i take it, will complete in a week?

@bvaughn
Copy link
Collaborator

@bvaughn bvaughn commented Oct 26, 2021

Sure.

@mdanyalkhan
Copy link

@mdanyalkhan mdanyalkhan commented Nov 23, 2021

Hi,

@bvaughn may I now take this? I have examined the code and I believe I've come up with a potential solution

@bvaughn
Copy link
Collaborator

@bvaughn bvaughn commented Nov 24, 2021

Sure.

@ibandim123
Copy link

@ibandim123 ibandim123 commented Nov 30, 2021

This problem has been resolved??

@mdanyalkhan
Copy link

@mdanyalkhan mdanyalkhan commented Dec 5, 2021

Hey! I've set up a PR to address the issue (SSR side for option elements) #22841, @bvaughn or anyone else in the React team, let me know your thoughts!

@michaelCHU95
Copy link

@michaelCHU95 michaelCHU95 commented Dec 28, 2021

@philipp-spiess @bvaughn Hi guys, what's the latest status of this PR? Is it still open? I was trying out for both <textarea> and , the invalid props warning seemed to work fine.

@AK1304
Copy link

@AK1304 AK1304 commented Jan 18, 2022

hey @philipp-spiess is this issue still unresolved , I am new to open source and I would like to give it a try

@AK1304
Copy link

@AK1304 AK1304 commented Jan 18, 2022

please let me know some good first issues that I can work on as I am new to open source

@cesarvargas00
Copy link

@cesarvargas00 cesarvargas00 commented Feb 3, 2022

I believe mdanyalkhan solved it in his PR #22841.

@its-kunal
Copy link

@its-kunal its-kunal commented Mar 14, 2022

@gaearon can I start working on this.

@EngNelson
Copy link

@EngNelson EngNelson commented Apr 9, 2022

Hello, I am Nelson, I am new here and I want to work on the first issue but I need someone to guide me.

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