Warn when input's value is null

[jimfb]

Passing null indicates that the user tried to specify a value (eg. from the database) but didn't notice the value was null; seems like that should be a warning. Normally we treat null and undefined as the same, but in this case passing null indicates an error and should therefore be discouraged. The user should decide if they want an uncontrolled component (in which case they should pass undefined) or if they want an empty controlled component (in which case they should pass the empty string).

@spicyj said he would be fine with this being a warning and/or with treating null as an empty string. Making it an error/warning has the advantage that there is now an easy upgrade path (fix the warning) without us introducing subtle changes in behavior that will break people's apps.

[sophiebits]

(see also #2533)

To be clear, we should treat null as the empty string eventually regardless of if we have a warning. Makes sense to preserve behavior for a release though.

[jimfb]

Yep, ok, :agree:.

[antsmartian]

@jimfb & @spicyj Hello all, I thought of giving the pull request for this issue; If my understanding is correct, you expect an warning message needs to be printed, when the input DOM element gets its value as null. Digging into the source-code, input element is mapped by ReactDOMInput javascript file. So if my understanding is correct, I need to check the value of input both in mountWrapper and updateWrapper to be null, if yes we just need to give the warning. Is it correct?

Sorry if its very noob question, thanks for your help.

[jimfb]

@antoaravinth Sounds roughly correct to me. You'll want to do the same thing for TextArea and Select, since they can also be controlled inputs.

[antsmartian]

@jimfb: Thanks for your response. Will give a PR probably in a day.

[jimfb]

@antoaravinth You may find this diff very helpful, since it's almost exactly what you're trying to do: https://github.com/facebook/react/pull/5032/files

For the warning message, we should say something like:

value prop on textarea should not be null. Consider using the empty string to clear the component or undefined for uncontrolled components.

Also, just fyi, we're in a code freeze 0.14 so your change will land when we're ready to merge 0.15 stuff. You're welcomed to take a stab at it now, but will likely need to rebase once we're ready to merge 0.15 stuff.

[antsmartian]

@jimfb : Thanks for your help. Will do the same and give an PR in a day or so. Will rebase if necessary for 0.15 release, that shouldn't be a problem.

[antsmartian]

@jimfb : Actually I made the changes for Input, and give a run on testcases, looks like this new change is breaking at least 8 cases. Out of which all share the same error message; One of the test case that got failed is posted below:

ReactDOMSelect › it should support server-side rendering with defaultValue
  - Error: Expected test not to warn. If the warning is expected, mock it out using spyOn(console, 'error'); and test that the warning occurs.
        at Console.newError [as error] (scripts/jest/test-framework-setup.js:10:29)
        at warning (node_modules/fbjs/lib/warning.js:45:17)
        at warnIfValueIsNull (src/renderers/dom/client/wrappers/ReactDOMSelect.js:51:5)
        at Object.ReactDOMSelect.mountWrapper (src/renderers/dom/client/wrappers/ReactDOMSelect.js:163:5)
        at ReactDOMComponent.Mixin.mountComponent (src/renderers/dom/shared/ReactDOMComponent.js:572:24)
        at src/renderers/dom/server/ReactServerRendering.js:47:25
        at ReactServerRenderingTransaction.Mixin.perform (src/shared/utils/Transaction.js:140:20)
        at Object.renderToString (src/renderers/dom/server/ReactServerRendering.js:44:24)
        at Spec.<anonymous> (src/renderers/dom/client/wrappers/__tests__/ReactDOMSelect-test.js:397:33)

[jimfb]

@antoaravinth That error is basically saying that the test wasn't expecting any warnings to fire, but some warnings were printed. You can either add some checks for the warnings (as I did in my linked diff above) or you can modify the test such that it doesn't warn (eg. update the test to pass undefined instead of null). Either way, we should have at least one test that verifies the warning fired correctly and at least one unit test which passes undefined and shows that the error does not fire.

[antsmartian]

@jimfb Thanks for your help, I have given a PR ( https://github.com/facebook/react/pull/5048/files ). Let me know if anything else needed from my side.

[jimfb]

Fixed in #5048 Thanks @antoaravinth!

[IndifferentDisdain]

So... why is this a thing? null is a perfectly acceptable initial value. For example, when creating a new object (initialized w/ default values from the server then passed to the component as props) in a form that requires address, Address Line 2 is often optional. I'm perfectly aware that it's null, and I may or may not add a value to it when completing the form.

I do the suggested workaround <input value={foo || ''} onChange={this.handleChange} />, but it just seems hackey. From what I can tell on v15.6.1, the only issue this causes is the console warning. I know I can just ignore the warning, but it's distracting when debugging actual errors.

[gaearon]

I think the first post is a bit misleading.

There's no problem with null as a value. The problem is React treats this as a flag to make an input uncontrolled. That turned out to be confusing.

So we added a warning to discourage existing users from passing null. This lets us safely change the behavior for future version to treat null as an empty string. We haven't done this yet but that's the plan, if I understand it right.

Maybe now is a good time to do this. Do you mind raising a new issue to discuss?

[IndifferentDisdain]

@gaearon will do, thx.

[agnihotriketan]

Using defaultValue attribute worked for me in case of onblur.

[IndifferentDisdain]

@agnihotriketan defaultValue does work fine, but it's intended for uncontrolled inputs. If you're using controlled inputs, this is a workaround at best, IMO.

[jedwards1211]

However we solve this issue, I really want to have a less hacky way of passing value={null} to a controlled input.