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

Fix “no onChange handler” warning to fire on falsy values ("", 0, false) too #12628

Merged
merged 21 commits into from Jul 17, 2018

Conversation

wherestheguac
Copy link
Contributor

@wherestheguac wherestheguac commented Apr 17, 2018

References issue #12477

Description

What is the current behavior?

(Bug / Inconsistency)

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

No Warning.

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

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

*Note: was also happening with truthy values that == false (ex “0”)

Steps

  • Check for falsey value prop
    • Add onChange handlers to tests

  • Check for falsey checked prop
    • Add onChange handlers to tests

  • Write new tests expecting warning with falsey value prop (& truthy val that == false)

  • Write new tests expecting warning with falsey checked prop

@wherestheguac wherestheguac changed the title [WIP] Falsey PropTypes not triggering “no onChange handler” warning Falsey PropTypes not triggering “no onChange handler” warning Apr 17, 2018
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't show this warning for null value because we already show a different warning for null value. So the user sees two warnings for <input value={null} /> which can be disorienting:

screen shot 2018-04-25 at 4 59 09 pm

Could you please change this to keep null from firing the second warning?

@wherestheguac
Copy link
Contributor Author

@gaearon yeah that makes total sense, I’ll add something to check for that tonight.

@wherestheguac
Copy link
Contributor Author

@gaearon I’ve added those checks, thanks for your patience 😄

@@ -936,14 +1000,18 @@ describe('ReactDOMInput', () => {

it('should warn if value is null', () => {
expect(() =>
ReactTestUtils.renderIntoDocument(<input type="text" value={null} />),
ReactTestUtils.renderIntoDocument(
<input type="text" value={null} onChange={emptyFunction} />,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like these changes are now unnecessary and can be reverted? We still expect one warning with null even if you don't pass onChange.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes gotcha 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good now!

it('should warn of no event listener with a falsey checked prop', () => {
expect(() =>
ReactTestUtils.renderIntoDocument(
<input type="checkbox" checked="false" />,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong. "false" is not falsy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the behavior is right but the test title is confusing.

@gaearon gaearon dismissed their stale review July 17, 2018 21:16

outdated

@gaearon gaearon force-pushed the falsy-proptypes-no-warning-bug branch from 118ee8e to 9cfbed6 Compare July 17, 2018 21:28
@gaearon
Copy link
Collaborator

gaearon commented Jul 17, 2018

I did some minor changes — reverted unnecessary test tweaks, and also fixed undefined to be treated as uncontrolled (no warning). Thank you for this!

@gaearon gaearon changed the title Falsey PropTypes not triggering “no onChange handler” warning Fix “no onChange handler” warning to fire on falsy values ("", 0, false) too Jul 17, 2018
@gaearon gaearon merged commit 171e0b7 into facebook:master Jul 17, 2018
@gaearon gaearon mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants