facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.65k stars 46.44k forks source link

Inaccurate warning when value props is set without onChange. #1118

Closed mattmccray closed 6 years ago

mattmccray commented 10 years ago

When you create an input component and set its value without providing an onChange handler, React will log this to the console:

"You provided a value prop to a form field without an onChange handler. This will render a read-only field. If the field should be mutable use defaultValue. Otherwise, set either onChange or readOnly. "

This isn't entirely accurate however. For example in my app I rely on event bubbling so that I can set a single onChange on the form component, thereby saving my sanity preventing me from having to add an onChange to every. single. field. (There are a lot in the app I'm working on)

syranide commented 10 years ago

@darthapo Two-way binding (valueLink) may be something you want to use instead, you can even implement your own this.linkState if the one provided (addon) isn't ideal for your case.

PS. Also, it's just a helpful warning, it's quite (OCD-)annoying that they can't be turned off though...

mattmccray commented 10 years ago

On a related note, I'm wondering... When making a custom control that needs to properly support onChange event bubbling, is there any good reason not to use:

var node= this.refs.control.getDOMNode(),
    fakeEvent= { target:node }
React.addons.TestUtils.Simulate.change( node, fakeEvent )

Or if there's no actual input element to propagate as the target, maybe:

var node= this.getDOMNode(),
    fakeEvent= { target:{ name:this.props.name, value:"NEWVALUE" } }
React.addons.TestUtils.Simulate.change( node, fakeEvent )
Evernight commented 10 years ago

Another problem with the same warning: Is there a specific reason not to allow onKeyUp and onKeyDown instead of onChange? I have value depending on the state of the component and want to intercept keyCodes to change it. Also I want a blinking cursor so readOnly is not an option. The workaround is to use empty function for onChange but would be nice to avoid it.

syranide commented 10 years ago

@Evernight It's only a helpful warning, it is not wrong not to have onChange. But yes, it is annoying that they can't be squelched, so they can end up burrying actually important warnings (or just generally be an annoyance).

sophiebits commented 10 years ago

@Evernight Can you post a simple example of what your code looks like? I wasn't under the impression that listening to keydown/keyup will work with controlled components. (By the way, how would you want to react if someone were to paste in text or move it around using the mouse, etc., in a way that doesn't cause key events?)

Evernight commented 10 years ago

@syranide Yeah, but it's nicer to have no warnings (especially at runtime) when there probably shouldn't be any.

@spicyj Here: http://jsfiddle.net/cf76B/5/ . It behaves exactly as I want it to, I am not exactly sure why it ignores the mouse events though.

sophiebits commented 10 years ago

@Evernight What do you mean, it ignores the mouse events? This is a strange case and you can write onChange={function() {}} to suppress the warning.

Evernight commented 10 years ago

@spicyj I mean I can't move text around or paste/delete the text using mouse. Probably some debugging would explain it but the current behavior suits my needs. Maybe the case is not typical indeed, I still can use empty function as a workaround.

sophiebits commented 10 years ago

@Evernight Yes, this is because you're not handling onChange. It sounds like you might want an uncontrolled component. See http://facebook.github.io/react/docs/forms.html.

syranide commented 9 years ago

This seems like a "wontfix" as it's only meant as a helpful warning, not an error.

mczepiel commented 9 years ago

How about extending support of implementing onInput or onChange? In my case I didn't want to wait for a field to lose focus so I didn't have an onChange (I did put in a no-op to evade the noisy warning though)

UPDATE: FWIW the most significant usage of input is on an <input type="range>

Actually disregard this Sorry, the onChange normalization you've done mitigates most of my usage of onInput from some legacy code.

ktei commented 9 years ago

I came across this issue today as well. This is a bit weird because all I want is to set initial input values based on this.state, so there seems no particular reason for me to provide onChange in such case. Is this going to be fixed? Or, does it mean I'm doing anything improper?

tirdadc commented 9 years ago

@ktei you might want to consider using defaultValue for that purpose.

ktei commented 9 years ago

@tirdadc thanks for the advice. That works.

alexmcmillan commented 9 years ago

I'm getting this warning with the following example:

<fieldset onChange={this.onChange}>
    <input type='radio' >{'Potato'}</input>
    <input type='radio' checked={true} >{'Carrot'}</input>
    <input type='radio' >{'Corn'}</input>
</fieldset>

Is this warning helping by indicating I'm doing something wrong? Surely this is a perfectly acceptable design pattern?

faber commented 9 years ago

Stumbled into this same issue, where I am using event bubbling to handle lots of controlled input change events from one parent (the form element). While I understand that it's just a helpful warning, it is triggered a huge number of times for big forms, so I used the following hack and PROBABLY A VERY EXTREMELY BAD IDEA to suppress it:

require('react/lib/LinkedValueUtils')
  .Mixin
  .propTypes
  .value = function() { return; };

Make sure you put that in a file named something like __ugly_monkeypatchHack__ so you don't forget its a bad idea!

Any ideas for how this could potentially be supported more officially, or if that's something worth doing? To avoid the mess of detecting if and how the controlled inputs are being handled correctly, maybe there can be a way to declare in your component that you understand the warning and don't need it?

idubinskiy commented 9 years ago

@syranide Unfortunately, it seems that React sometimes goes a little bit overboard with the "helpful warnings". This is one of those cases.

waldreiter commented 9 years ago

This warning is very often helpful for beginners.

Those who know that they really don't want an onChange handler, why don't they just use a custom component? React is all about the composition of components. Maybe an example for a custom component should be added to the Forms docs?

maxmarchuk commented 8 years ago

I was having this problem too due to passing a function as a property to a component and binding that function to an input field's onChange value. To remove the warning, I just added a function inside of the component that calls the function in the component's props. e.g. <input ... onChange={this.props.onChange} ... /> to

onChange: function(e){
     e.preventDefault();
   this.props.onChange();
 },

...

<input onChange={this.onChange
andriichumak commented 8 years ago

This is extremely annoying. Having single onChange handler on form and using bubbling is a common pattern, specially for big forms. And having console full of warnings because of this is very strange. Basically, React is throwing warning at you when you're using something from "best practice" book.

syranide commented 8 years ago

React is throwing warning at you when you're using something from "best practice" book.

This is my opinion, but I'd say it's the opposite, it's a bad idea. It relies on bubbling outside of the React hierarchy. It's not portable and it's not something you find in other UI frameworks and not something you could reimplement yourself. Again, entirely my opinoin, but I even wish React would prevent this from being possible, it's a source of a pain and goes against the spirit of React.

Guria commented 8 years ago

@syranide

Like all DOM events, the onChange prop is supported on all native components and can be used to listen to bubbled change events.

https://facebook.github.io/react/docs/forms.html#interactive-props

onChange bubbling appears inside of the React hierarchy since it's not native, but introduced with React if I got it right.

NameFILIP commented 8 years ago

Having the same issue. Example:

<form onChange={props.onChange}>
  <input type="text" value={props.firstName} />
  <input type="text" value={props.lastName} />
  ...
</form>
gasipari commented 8 years ago

Any solution? I have the same issue <form onSubmit={this.handleSubmit}><input type="text" ref="todoText" placeholder="Enter a task" />...</form>

miclaus commented 8 years ago

+1

ashutosh-s commented 8 years ago

Same issue here .

in my case value comes from autopopulted username & email fields

warning.js:44 Warning: Failed form propType: You provided a `value` prop to a form field without an `onChange` handler. This will render a read-only field. If the field should be mutable use `defaultValue`. Otherwise, set either `onChange` or `readOnly`. Check the render method of `Header`.
mor-gilad commented 8 years ago

What about adding: onChange={()=>{}} as an input attribute? It's not perfect but it helps with the "warnings OCD" :)

miclaus commented 8 years ago

defaultValue did the job for me:

<input
  ref={(ref) => this.SignupEmailInput = ref}
  type="email"
  className="input-field"
  placeholder="email address"
  defaultValue={this.props.userData.email}
/>

! Note @syranide's note below:

But then it's no longer controlled.

d-alejo90 commented 8 years ago

Thanks! @miclaus It worked for me too!

syranide commented 8 years ago

@miclaus But then it's no longer controlled.

thymikee commented 8 years ago

@syranide If you want it to be controlled and surpass the warning, you can go with setting value normally:

<input
  ...
  onChange={props.onChange}
  value={props.value}
/>

and make use of Component's defaultProps property:

YourComponent.defaultProps = { value: '' };

(no idea if it's documented somewhere). Hope that helps.

kelabang commented 8 years ago

it works !

noahmonster commented 8 years ago

Just wanted to leave another use-case where this can happen.

I'm using a custom CSS checkbox where the a click handler needs to be on the text and the checkbox.

<div className="special-checkbox" onClick={this.handleClick}>
  <input type="checkbox" checked={this.state.isChecked}>
  <span>{this.props.label}</span>
</div>

It looks like this: screen shot 2016-08-24 at 2 58 47 pm

The state is correctly stored on the input, but the user-agent style for checkboxes is set to display:none so the change handler doesn't work for the actual input because it's not clickable.

faalsh commented 7 years ago

I came across a different use case for the same issue. I have a checkbox wrapped in a div and decided to change the state of the checkbox with onClick event instead of onChange because onClick event of the div element will be executed before onChange event.

My component looks something like this:

<div onClick={this.doSomething}>

............

<input type='checkbox' checked={checked} onClick={this.doSomethingElse} />

.........
</div>

In the above example, doSomethingElse will be executed first, then i will stop propagating the event.

However, if I used onChange instead of onClick in the checkbox element, doSomething will be executed before doSomethingElse, and in this case I will not be able to stop propagating the event.

Here is an example https://jsfiddle.net/69z2wepo/74406/

My point is: if onClick is provided in checkbox input, there should not be a warning

Negan1911 commented 7 years ago

Yeah i have the same issue and i think that we should have a way to turn this warning of, or remove it, because it is not an "helpful" message but an error represented on the console.

tswaters commented 7 years ago

This will also raise this warning... when a checkboxes (or any input really) does not control its own checked/value attribute.... you might have any arbitrary element that controls it.

<button aria-controls="checkbox" onClick={() => this.setState({checked: !this.state.checked})}>{'Check it!'}</button>
<input id="checkbox" type="checkbox" checked={this.state.checked} />

While I can see this being valuable for newbies - these edge cases just add noise to the logs in other cases.... that said, it would be neigh impossible to come up with a propType for this that was accurate / useful 100% of the time.

Also, fwiw - the checked warning for checkboxes only shows if the value of checked is true - if the element is not checked, it doesn't raise the warning.... probably a separate issue.

syranide commented 7 years ago

@tswaters In your case the checkbox should be disabled or readOnly.

Epyon616 commented 7 years ago

This is a particularly annoying warning message, especially in my case, as I am passing an onChange handler and everything looks and works perfectly in development but when I try testing my form component the tests are riddled with these warnings which I'm pretty sure are masking the crux of the problem I'm trying to solve.

Whats more adding onChange={() => {}} is pointless in my case since I am trying to test and onChange handler that is being passed to a text field, and while this does remove the warning it still means I can't write tests for the handler.

drewlustro commented 7 years ago

+1 on this issue.

I had to attach a bogus onChange={() => {}} because grommet's TextInput component provides controlled management through the custom prop onDOMChange.

If I provide a value prop, React complains by printing this warning even though the input is, in fact, controlled.

dougkl commented 7 years ago

I had the same problem.... Instead value=, use the defaultValue input type="text" name="mokedvalue" defaultValue={this.myvalue} onBlur={this.updatemyvalue.bind(this)} onWheel={this.onWheel}

gaearon commented 6 years ago

If the only reason you don’t want to put onChange on the input itself is because you have to create many handlers, that’s not really true. Here is an example of using a single event handler for multiple controlled inputs.

In other cases, if you really want to “break out” of React’s model, you can use uncontrolled components as an escape hatch. They won’t show this warning.

If you have a use case for which neither of those options works well, please file a new issue and describe it in more detail.

turnerhayes commented 6 years ago

I have the same sort of issue, where I'm handling value changes in onKeyDown. I can fix it by adding a no-op onChange handler, as mentioned above, but that seems absurd just to work around this (inaccurate) warning. I'm not sure how an uncontrolled component would really work in this case, either; I mean, I guess I could add a handler using vanilla JS on the ref element, but that sounds very dangerous.

scabbiaza commented 6 years ago

In other cases, if you really want to “break out” of React’s model, you can use uncontrolled components as an escape hatch. They won’t show this warning.

Uncontrolled components limit you to the basic HTML form elements. As soon as you need to implement slider / picker on top of DIV and custom markup – you need to make it controlled.

In my case I really want to "break out" of React's model (I use CycleJS-like architecture with declarative stream-based events) and I'd really wish to disable that warning.

sophiebits commented 6 years ago

As soon as you need to implement slider / picker on top of DIV and custom markup – you need to make it controlled.

@scabbiaza I don't believe this is true. Can you clarify what you mean?

ivan-kleshnin commented 6 years ago

@scabbiaza I don't believe this is true. Can you clarify what you mean?

Uncontrolled component has its state controlled by Browser, stored in DOM. div (as slider, e.g) doesn't have a DOM state so its "state" should be managed externally.

You can't make an "uncontrolled div" because browser applies no default behavior to divs. However you can emulate any UI element with div(s) by making it controlled. It won't be controlled in exactly the same sense as input (as div has no value) but it will be controlled in the architectural sense (DOM tied to some external data source). So this arguing does not directly apply to the topic and was made to counter-argue the quoted comment, I guess.

to break of React's model use uncontrolled components

it's not always possible or desired. Uncontrolled components are limited to built-in UI elements. And with controlled components, bubbling and other totally fine architectural patterns which React "doesn't favor" are screwed by this warning.

I believe warning should either be 100% to the point, otherwise it's better be removed. I personally see a disturbing trend to add more and more warnings to React, in parallel with more and more requests of options to disable them.

This particular warning is false in many cases, belongs to documentation area and should be removed i.m.o.

rawpixel-vincent commented 6 years ago

Why don't convert this warning to a yellow notice, since not having onChange on every input is valid when you rely on event bubbling. This warning end up hiding other warnings, since it always appear it makes other warnings less visible.

s7dhansh commented 6 years ago

How about using defaultValue with a random key prop, eg: <input defaultValue={props.value} key={Math.random()}/>

What are the pitfalls? Will the performance drain be noticeable for a large form?

selbekk commented 6 years ago

Here's a reproducable example: https://codesandbox.io/s/xrnl20np7w

corysimmons commented 5 years ago

If the only reason you don’t want to put onChange on the input itself is because you have to create many handlers, that’s not really true. Here is an example of using a single event handler for multiple controlled inputs.

You still have to attach onChange={someHandler} to every input (which can be a lot of inputs depending on your business needs).

Whereas...

<form onChange={e => {
  e.preventDefault()
  changeFormValues({
    ...oldFormValues,
    [e.target.id]: e.target.value
  })
}}>
  <input type="text" id="foo" />
</form>

...can/should work just fine without this error.

I understand the desire to keep warnings in for newbs (although no one will get far in React unless they grok the whole controlled input thing...), but defaultValue seems like it should be a perfect escape hatch for this?

Either way, I run into this semi-regularly and don't want to put onChange on every <input> or use one of the aforementioned hacks here.

Here it is throwing errors with the trending Easy Peasy library: https://codesandbox.io/s/easy-peasy-playground-with-react-warning-e2wug

At the very least could we add something to ESLint or something to disable this warning?

corysimmons commented 5 years ago

@gaearon @sophiebits I know you're busy, but friendly ping just to put this back on your radar in hopes you'll reconsider removing this warning or at least giving us a way to opt out of it.

I think a lot of people would really like to be able to do <form onChange /> instead of

<input onChange />
<input onChange />
<input onChange />

Thank you for all your hard work.