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

Warn if checkbox changes controlledness #7544

Closed
jimfb opened this issue Aug 23, 2016 · 5 comments
Closed

Warn if checkbox changes controlledness #7544

jimfb opened this issue Aug 23, 2016 · 5 comments

Comments

@jimfb
Copy link
Contributor

jimfb commented Aug 23, 2016

We should be warning if a controlled component becomes uncontrolled, or vice versa.

I thought this was fixed in #5821, but apparently it was either not fixed for checkbox components, or maybe I subsequently broke it in #7003 (or similar). Regardless, we should fix it.

Demo of case that should warn: https://jsfiddle.net/s4742gkk/

@marcin-mazurek
Copy link
Contributor

I'd like to give it a try. Is there anything else I need to know before I pick this up?

@jimfb
Copy link
Contributor Author

jimfb commented Aug 23, 2016

@marcin-mazurek Sounds good. I'd recommend looking at the issues I linked to collect any background and/or ideas. Also, we should add a couple of unit tests to verify the new functionality and make sure we don't regress it again.

@marcin-mazurek
Copy link
Contributor

marcin-mazurek commented Aug 26, 2016

It seems like determining controlledness is not consistent. The validation treats component as uncontrolled only when checked is undefined, however rendered checkbox/radio can be toggled also when checked is null.

https://facebook.github.io/react/tips/controlled-input-null-value.html - this page suggests that null is considered a uncontrolled value, but this is a text input. No mention about a checkbox/radio input.

What do you think should happen when checkbox has checked={null}?
If we want to warn when changing from null to boolean (or the other way round), the solution would look like this: marcin-mazurek@5cf453f

It's just proof of concept, not the final solution. The final solution requires some more work to implement consistent solution everywhere.

At present, when switching from a controlled value of a text input, we're getting following warning:

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

This comes from src/renderers/dom/shared/hooks/ReactDOMNullInputValuePropHook.js. Maybe we should implement similar warning for checkboxes - as null is a bit unexpected value for checked - I'd guess only undefined or boolean value should be allowed.

@jimfb
Copy link
Contributor Author

jimfb commented Aug 26, 2016

What do you think should happen when checkbox has checked={null}?

Should behave exactly as setting it to undefined does.

If we want to warn when changing from null to boolean (or the other way round), the solution would look like this: marcin-mazurek/react@5cf453f

Yes, that looks approximately correct to me. Left an inline comment.

This comes from src/renderers/dom/shared/hooks/ReactDOMNullInputValuePropHook.js. Maybe we should implement similar warning for checkboxes - as null is a bit unexpected value for checked - I'd guess only undefined or boolean value should be allowed.

Yeah, probably true. Historically we've generally tried to treat null and undefined as the same, but I agree that passing null almost always indicates a bug. I don't have a strong opinion here.

@marcin-mazurek
Copy link
Contributor

marcin-mazurek commented Aug 29, 2016

Hey @jimfb. Take a look at #7603 please.
Regarding your comment - I used != null rather than != undefined as comparing undefined with equality operator is forbidden by linter.

As I mentioned in the PR, there aren't any warnings for changing controlledness for textarea and select. Is this something we should improve?

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

2 participants