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
Member

@gaearon gaearon left a comment

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?

Loading

@wherestheguac
Copy link
Contributor Author

@wherestheguac wherestheguac commented Apr 25, 2018

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

Loading

@wherestheguac
Copy link
Contributor Author

@wherestheguac wherestheguac commented May 16, 2018

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

Loading

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

@gaearon gaearon May 16, 2018

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.

Loading

Copy link
Contributor Author

@wherestheguac wherestheguac May 17, 2018

Choose a reason for hiding this comment

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

Ah yes gotcha 👍

Loading

Copy link
Member

@gaearon gaearon May 23, 2018

Choose a reason for hiding this comment

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

Ping :-)

Loading

Copy link
Contributor Author

@wherestheguac wherestheguac Jun 3, 2018

Choose a reason for hiding this comment

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

Good now!

Loading

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

@gaearon gaearon Jul 17, 2018

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.

Loading

Copy link
Member

@gaearon gaearon Jul 17, 2018

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.

Loading

@gaearon gaearon dismissed their stale review Jul 17, 2018

outdated

@gaearon gaearon force-pushed the falsy-proptypes-no-warning-bug branch from 118ee8e to 9cfbed6 Jul 17, 2018
@gaearon
Copy link
Member

@gaearon 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!

Loading

@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
1 check was pending
Loading
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
…se) too (facebook#12628)

* throw warning for falsey `value` prop

* add nop onChange handler to tests for `value` prop

* prettier

* check for falsey checked

* fix tests for `checked` prop

* new tests for `value` prop

* test formatting

* forgot 0 (:

* test for falsey `checked` prop

* add null check

* Update ReactDOMInput-test.js

* revert unneeded change

* prettier

* Update DOMPropertyOperations-test.js

* Update ReactDOMInput-test.js

* Update ReactDOMSelect-test.js

* Fixes and tests

* Remove unnecessary changes
@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
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants