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

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

Closed
mattmccray opened this issue Feb 18, 2014 · 51 comments
Closed

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

mattmccray opened this issue Feb 18, 2014 · 51 comments

Comments

@mattmccray
Copy link

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

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

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

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

syranide commented Mar 3, 2014

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

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

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

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

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

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

syranide commented Oct 8, 2014

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

@mczepiel
Copy link

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

ktei commented Dec 26, 2014

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

tirdadc commented Dec 30, 2014

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

@ktei
Copy link

ktei commented Dec 31, 2014

@tirdadc thanks for the advice. That works.

@alexmcmillan
Copy link

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

faber commented Mar 2, 2015

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

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

@waldreiter
Copy link
Contributor

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?

bilus added a commit to bilus/reforms that referenced this issue Jun 20, 2015
@maxmarchuk
Copy link

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

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

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

Guria commented Apr 22, 2016

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

NameFILIP commented May 17, 2016

Having the same issue. Example:

<form onChange={props.onChange}>
  <input type="text" value={props.firstName} />
  <input type="text" value={props.lastName} />
  ...
</form>

@gasipari
Copy link

gasipari commented May 25, 2016

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

@miclaus
Copy link

miclaus commented Jun 13, 2016

+1

@ashutosh-s
Copy link

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

mor-gilad commented Jun 14, 2016

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

@kelabang
Copy link

it works !

@noahmonster
Copy link

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

faalsh commented Mar 23, 2017

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

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

tswaters commented May 25, 2017

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

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

@Epyon616
Copy link

Epyon616 commented Jul 17, 2017

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

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

dougkl commented Aug 2, 2017

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

gaearon commented Oct 1, 2017

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.

@gaearon gaearon closed this as completed Oct 1, 2017
@turnerhayes
Copy link

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

scabbiaza commented Nov 29, 2017

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

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

ivan-kleshnin commented Nov 30, 2017

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

rawpixel-vincent commented Mar 28, 2018

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

s7dhansh commented Apr 1, 2018

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

selbekk commented Jul 19, 2018

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

@corysimmons
Copy link

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

@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.

@leidegre
Copy link
Contributor

leidegre commented Jan 29, 2021

@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.

6 years late but essentially this!

<input
  type="url"
  value={value}
  onChange={suppressMissingOnChangeHandlerWarning}
  onBlur={e => onChange(e.currentTarget.value)}
/>

Works great!


In case it is unclear, suppressMissingOnChangeHandlerWarning is just a function declared elsewhere so the prop is stable and sort of documented.

function suppressMissingOnChangeHandlerWarning() {}

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

No branches or pull requests