Issues with ReactControlledValuePropTypes

[lukescott]

Do you want to request a feature or report a bug?

See below.

What is the current behavior?

1. (Bug / Inconsistency)

<input type="radio" checked={false} />

No Warning.

2. (Feature Request)

<input type="radio" checked={true} onChange={undefined} />

Warning: Failed prop type: You provided a 'checked' prop to a form field without an 'onChange' handler. This will render a read-only field. If the field should be mutable use 'defaultChecked'. Otherwise, set either 'onChange' or 'readOnly'.'

3. (Bug?)

<select value="foo" readOnly={true}>...</select>

No warning.

What is the expected behavior?

1. Passing a falsy value or checked attribute will not trigger a warning, but a truthy value does.

2. If onChange is passed as undefined (or null?) this should be considered as an acknowledgement and silence the warning. I have a case where I split the render method from the component and use it as a preview. When the component is interactive I use the component. When doing a preview I pass undefined as my change handler. A warning is shown to tell me I "forgot" it, but I intended it to be this way.

3. Going along with 2, I can pass readOnly={!handleChange}, but according to DefinitelyTyped this isn't a valid attribute for select. I can't use disabled because it changes the appearance of the field.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

16.2.0
Chrome 65

Other

A proposed "fix" would be to change:

react/packages/react-dom/src/shared/ReactControlledValuePropTypes.js

Lines 27 to 33 in 37e4329

	if (
	!props[propName] ||
	hasReadOnlyValue[props.type] ||
	props.onChange ||
	props.readOnly ||
	props.disabled
	) {

to:

if ( 
    !(propName in props) || // Fixes 1
    hasReadOnlyValue[props.type] || 
    "onChange" in props ||  // Fixes 2
    props.readOnly || 
    props.disabled 
 ) { 

And:

react/packages/react-dom/src/shared/ReactControlledValuePropTypes.js

Lines 44 to 49 in 37e4329

	if (
	!props[propName] ||
	props.onChange ||
	props.readOnly ||
	props.disabled
	) {

to:

if (
    !(propName in props[propName]) || // Fixes 1
    "onChange" in props || // Fixes 2
    props.readOnly ||
    props.disabled
) {

[gaearon]

Can you please compare behavior in 15 and 16.3.0 (next tag for react and react-dom on npm)?
Has this changed at some point?

[lukescott]

Just checked both 15 and 16.3 - Same behavior for both.

I didn't discover this until I hit a field that had an initial truthy state (radios, select boxes). We have an application where a user builds a form and we use a stateless render function for preview and then call that function within a class component for the usable field. I would like to avoid using readOnly because it would only be valid in the preview case and adds a bit of confusion for our other developers. It also doesn't seem to be supported on selects (at least in TypeScript's type def).

[wherestheguac]

Hey! I’ve started to work on this here #12601

Any input is appreciated 😄

[gaearon]

If onChange is passed as undefined (or null?) this should be considered as an acknowledgement and silence the warning. I have a case where I split the render method from the component and use it as a preview. When the component is interactive I use the component. When doing a preview I pass undefined as my change handler. A warning is shown to tell me I "forgot" it, but I intended it to be this way.

So, I don't quite agree with this. I see what you're saying but there's also a legitimate case where you write onChange={this.handleCahnge} and get undefined because you made a typo (did you notice mine?)

But if the field is suddenly readonly that's really confusing. I think the case with a typo is more common than the intentional use case you're explaining. And in your case, you could explicitly pass a noop function that doesn't do anything to silence the warning.

[lukescott]

If I passed this.handleCahnge instead of this.handleChange my linter would catch it. So the question is: Is the warning intended to catch typos, or that I did not provide an onChange at all? IMO, if I specify onChange and my variable is undefined (typo or not) it should be an acknowledgement.

An alternative could be to allow null. I'm not too keen on creating nop for every case, importing it, or having a dependency for one.

[jquense]

I don't know that I see the value in the null or undefined sigil here when you can always do: onChange={readOnlyHandler} where readOnlyHandler= () => {}

[lukescott]

It's a warning. It functions as read-only with the warning. The warning is intended to let you know that you need an onChange handler w/ value. But by passing undefined, I'm acknowledging that. IMO, having the property set is significant enough vs not specifying it at all.

Likewise checked={false} should be treated the same as checked={true}: If onChange is not specified at all a warning should be emitted in either case, whereas currently it does not.

It both cases it should not be looking at the value, but rather if the prop was passed at all ("name" in props vs !props.name). Falsy checks are imprecise.

[gaearon]

my linter would catch it.

Then you have a very good linter 🙂. I’m not aware of built-in ESLint rules that catch a mistake like this.

We also try to not make assumptions about the user’s toolset. We try to warn about typos even when a linter is capable of catching it.

I'm not too keen on creating nop for every case, importing it, or having a dependency for one.

This seems like a philosophical stance. I don’t see a practical argument here. As a dependency it’s literally a few bytes. It’s also very low effort to type onChange={() => {}}. It’s even shorter than onChange={undefined}. And gives you an opportunity to note the reason in a comment.

<input onChange={() => {
  // intentionally empty because reasons
}} />

Passing a falsy value or checked attribute will not trigger a warning, but a truthy value does.

This does look like a bug to me.

[lukescott]

Maybe it would help with a more specific code example. This is what my render function looks like:

export function render({
	label,
	placeholder = "",
	value = "",
	maxLength,
	error,
	handleChange,
}: TextFieldProps & Partial<TextFieldEvents>) {
	const classes: string[] = ["textField"]
	if (error) {
		classes.push(error)
	}
	return (
		<div className={classes.join(" ")}>
			<label>{label}</label>
				<input
					type="text"
					placeholder={placeholder}
					value={value}
					onChange={handleChange}
					maxLength={maxLength} />
		</div>
	)
}

I have a supervising PureComponent that passes handleChange. Without the component I use the render function by itself for read-only previews.

In this case I do not get a warning because the "" is falsy. I did not encounter this warning until I got to a more complicated case where the default value was truthy (radio group). If I had gotten the warning on a falsy value it would have alerted me to the issue sooner.

I could do something like this to fix it:

export function render({
	label,
	placeholder = "",
	value = "",
	maxLength,
	error,
	handleChange = () => undefined,
}: TextFieldProps & Partial<TextFieldEvents>) {

Which would be easy to do. But... wait, you aren't supposed to inline functions, are you? In this case it causes a re-render even though nothing has really changed.

So I would need to cache it first:

const nop = () => undefined

export function render({
	label,
	placeholder = "",
	value = "",
	maxLength,
	error,
	handleChange = nop,
}: TextFieldProps & Partial<TextFieldEvents>) {

I know this is a bit nit-picky, but I would rather allow undefined (or null) to use as a nop than a blank method. If someone else on my team came across this they would have done what I initially did without knowing they cause a rendering issue.

[gaearon]

#12628 fixed the part that was buggy. Thanks for the report.