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

Warning on <input value={null} /> #6996

Closed
sximba opened this Issue Jun 8, 2016 · 2 comments

Comments

Projects
None yet
3 participants
@sximba

sximba commented Jun 8, 2016

@syranide @jimfb

This issue of <input /> vs <input value={null} /> has been around for over a year now but I still can't understand the thinking behind the reasoning you give to all the issues that raise it.

I apologise if the following indicates I am misunderstanding something. Please clarify for me what I seem to not understand.

Could you please give an example of where a developer may use <input value={null} /> for an uncontrolled input.

Because to me it seems React only "sees" such if a developer did something like <input value={x} /> and x happens to be null (let x = null). This, in terms of intention, means x may be a non-null value later. This should leave the component as a controlled component. My understanding is that React already does this. My issue is about the warning it gives.

Currently, React 15.x.x gives a warning when it encounters and it says the value should not be null but rather an empty string or undefined, depending on whether you want a controlled vs uncontrolled input. The release notes for v15.0.0 say React treats this as an intention to clear the input. Can this warning be dropped because it may lead developers to having let value = value || "" all over their code, especially if using async store patterns where values may get set at a later point. This leads to the mistake of losing a value of 0 in some instances (or all).

To me it seems like this is something that should be in docs and not a warning in code. I have yet to see a case where a developer uses the value prop on an uncontrolled input. This is a direct result of lack of cases where a controlled input is changed to an uncontrolled input. In that case, Reach should encourage developers to be more explicit in their intention by having some flow control mechanism that returns a different component altogether instead of setting the same controlled input to null.

@syranide

This comment has been minimized.

Contributor

syranide commented Jun 8, 2016

This my my opinion and I'm a big proponent of being strict, being lenient may be nice but it enables error prone behaviors, allows otherwise easily detected bugs to go into production and complicates debugging.

First off, the root problem is that controlled and uncontrolled components are the same. They should be separate components. You should never switch between the two. To manage controlledness by the existence of value or it being null are both weird in their own ways. You can argue for either but changing it now would cause major head ache and still not fix the situation.

However, I'm still not saying that providing null and internally coercing it to '' is a good idea even with separate components, because it's weird in its own way. You can then have a controlled input with value null and as soon as you change it you can never go back to it being null, not even if you undo... there's something funky about that. It also means that your internal state may report null or '' (despite being visually identical) as the result to something else if you don't coerce it anyway, you may have a test which tests for v === '' which would then succeed, despite it being empty. It may be handy to have this behavior, but it's dirty. In more complex cases it's even arbitrary and ambiguous, what about number inputs, should it then be '' or 0? Etc.

You're free to implement your own component that does whatever you want internally. You may even go as far as creating async-aware inputs that prevent you from modifying them while they are being loaded (represented as null or whichever way you prefer), you can improve the behavior to suit your project. But the default should be to tell you when you're doing something generally bad.

This should leave the component as a controlled component. My understanding is that React already does this. My issue is about the warning it gives.

It does not.

Can this warning be dropped because it may lead developers to having let value = value || "" all over their code, especially if using async store patterns where values may get set at a later point. This leads to the mistake of losing a value of 0 in some instances (or all).

IMHO that's not a bad thing, 0 becoming '' is, but that's solved by value != null ? value : '', you can hide it behind a helper if you prefer ifNull(value, ''). Yes it's not terribly nice, but neither is passing around null when you should be passing strings. Just because an async callback is in progress doesn't mean that "empty/default" is a valid substitute, be explicit. JavaScript many times allowing strings in-place of numbers is another one of these, just because you can doesn't mean you should, it means your code may break unexpectedly in unrelated parts of the code.

Reach should encourage developers to be more explicit in their intention by having some flow control mechanism that returns a different component altogether instead of setting the same controlled input to null.

Agreed, they should be separate.

@jimfb

This comment has been minimized.

Contributor

jimfb commented Jun 8, 2016

Thanks @syranide, great answer!

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