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
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -9,6 +9,8 @@

'use strict';

const emptyFunction = require('fbjs/lib/emptyFunction');

describe('DOMPropertyOperations', () => {
let React;
let ReactDOM;
Expand Down Expand Up @@ -79,7 +81,7 @@ describe('DOMPropertyOperations', () => {

it('should not remove empty attributes for special properties', () => {
const container = document.createElement('div');
ReactDOM.render(<input value="" />, container);
ReactDOM.render(<input value="" onChange={emptyFunction} />, container);
expect(container.firstChild.getAttribute('value')).toBe('');
expect(container.firstChild.value).toBe('');
});
Expand Down
127 changes: 106 additions & 21 deletions packages/react-dom/src/__tests__/ReactDOMInput-test.js
Expand Up @@ -36,6 +36,30 @@ describe('ReactDOMInput', () => {
ReactTestUtils = require('react-dom/test-utils');
});

it('should warn of no event listener with a falsey value of 0', () => {
expect(() => {
ReactTestUtils.renderIntoDocument(<input type="text" value={0} />);
}).toWarnDev(
'Failed prop type: You provided a `value` prop to a form field without an `onChange` handler.',
);
});

it('should warn of no event listener with a falsey value of ""', () => {
expect(() => {
ReactTestUtils.renderIntoDocument(<input type="text" value="" />);
}).toWarnDev(
'Failed prop type: You provided a `value` prop to a form field without an `onChange` handler.',
);
});

it('should warn of no event listener with a value of "0"', () => {
expect(() => {
ReactTestUtils.renderIntoDocument(<input type="text" value="0" />);
}).toWarnDev(
'Failed prop type: You provided a `value` prop to a form field without an `onChange` handler.',
);
});

it('should properly control a value even if no event listener exists', () => {
const container = document.createElement('div');
let stub;
Expand Down Expand Up @@ -475,7 +499,7 @@ describe('ReactDOMInput', () => {
});

it('should display `value` of number 0', () => {
let stub = <input type="text" value={0} />;
let stub = <input type="text" value={0} onChange={emptyFunction} />;
stub = ReactTestUtils.renderIntoDocument(stub);
const node = ReactDOM.findDOMNode(stub);

Expand Down Expand Up @@ -625,8 +649,14 @@ describe('ReactDOMInput', () => {
it('should properly transition from an empty value to 0', function() {
const container = document.createElement('div');

ReactDOM.render(<input type="text" value="" />, container);
ReactDOM.render(<input type="text" value={0} />, container);
ReactDOM.render(
<input type="text" value="" onChange={emptyFunction} />,
container,
);
ReactDOM.render(
<input type="text" value={0} onChange={emptyFunction} />,
container,
);

const node = container.firstChild;

Expand All @@ -637,8 +667,14 @@ describe('ReactDOMInput', () => {
it('should properly transition from 0 to an empty value', function() {
const container = document.createElement('div');

ReactDOM.render(<input type="text" value={0} />, container);
ReactDOM.render(<input type="text" value="" />, container);
ReactDOM.render(
<input type="text" value={0} onChange={emptyFunction} />,
container,
);
ReactDOM.render(
<input type="text" value="" onChange={emptyFunction} />,
container,
);

const node = container.firstChild;

Expand All @@ -649,8 +685,14 @@ describe('ReactDOMInput', () => {
it('should properly transition a text input from 0 to an empty 0.0', function() {
const container = document.createElement('div');

ReactDOM.render(<input type="text" value={0} />, container);
ReactDOM.render(<input type="text" value="0.0" />, container);
ReactDOM.render(
<input type="text" value={0} onChange={emptyFunction} />,
container,
);
ReactDOM.render(
<input type="text" value="0.0" onChange={emptyFunction} />,
container,
);

const node = container.firstChild;

Expand All @@ -661,8 +703,14 @@ describe('ReactDOMInput', () => {
it('should properly transition a number input from "" to 0', function() {
const container = document.createElement('div');

ReactDOM.render(<input type="number" value="" />, container);
ReactDOM.render(<input type="number" value={0} />, container);
ReactDOM.render(
<input type="number" value="" onChange={emptyFunction} />,
container,
);
ReactDOM.render(
<input type="number" value={0} onChange={emptyFunction} />,
container,
);

const node = container.firstChild;

Expand All @@ -673,8 +721,14 @@ describe('ReactDOMInput', () => {
it('should properly transition a number input from "" to "0"', function() {
const container = document.createElement('div');

ReactDOM.render(<input type="number" value="" />, container);
ReactDOM.render(<input type="number" value="0" />, container);
ReactDOM.render(
<input type="number" value="" onChange={emptyFunction} />,
container,
);
ReactDOM.render(
<input type="number" value="0" onChange={emptyFunction} />,
container,
);

const node = container.firstChild;

Expand Down Expand Up @@ -911,6 +965,16 @@ describe('ReactDOMInput', () => {
ReactTestUtils.Simulate.change(instance);
});

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.

),
).toWarnDev(
'Failed prop type: You provided a `checked` prop to a form field without an `onChange` handler.',
);
});

it('should warn with checked and no onChange handler with readOnly specified', () => {
ReactTestUtils.renderIntoDocument(
<input type="checkbox" checked="false" readOnly={true} />,
Expand All @@ -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!

),
).toWarnDev(
'`value` prop on `input` should not be null. ' +
'Consider using an empty string to clear the component or `undefined` ' +
'for uncontrolled components.',
);

ReactTestUtils.renderIntoDocument(<input type="text" value={null} />);
ReactTestUtils.renderIntoDocument(
<input type="text" value={null} onChange={emptyFunction} />,
);
});

it('should warn if checked and defaultChecked props are specified', () => {
Expand Down Expand Up @@ -1053,7 +1121,10 @@ describe('ReactDOMInput', () => {
const container = document.createElement('div');
ReactDOM.render(stub, container);
expect(() =>
ReactDOM.render(<input type="text" value="controlled" />, container),
ReactDOM.render(
<input type="text" value="controlled" onChange={emptyFunction} />,
container,
),
).toWarnDev(
'Warning: A component is changing an uncontrolled input of type text to be controlled. ' +
'Input elements should not switch from uncontrolled to controlled (or vice versa). ' +
Expand All @@ -1064,7 +1135,7 @@ describe('ReactDOMInput', () => {
});

it('should warn if uncontrolled input (value is null) switches to controlled', () => {
const stub = <input type="text" value={null} />;
const stub = <input type="text" value={null} onChange={emptyFunction} />;
const container = document.createElement('div');
expect(() => ReactDOM.render(stub, container)).toWarnDev(
'`value` prop on `input` should not be null. ' +
Expand Down Expand Up @@ -1140,7 +1211,10 @@ describe('ReactDOMInput', () => {
const container = document.createElement('div');
ReactDOM.render(stub, container);
expect(() =>
ReactDOM.render(<input type="checkbox" checked={true} />, container),
ReactDOM.render(
<input type="checkbox" checked={true} onChange={emptyFunction} />,
container,
),
).toWarnDev(
'Warning: A component is changing an uncontrolled input of type checkbox to be controlled. ' +
'Input elements should not switch from uncontrolled to controlled (or vice versa). ' +
Expand All @@ -1151,11 +1225,16 @@ describe('ReactDOMInput', () => {
});

it('should warn if uncontrolled checkbox (checked is null) switches to controlled', () => {
const stub = <input type="checkbox" checked={null} />;
const stub = (
<input type="checkbox" checked={null} onChange={emptyFunction} />
);
const container = document.createElement('div');
ReactDOM.render(stub, container);
expect(() =>
ReactDOM.render(<input type="checkbox" checked={true} />, container),
ReactDOM.render(
<input type="checkbox" checked={true} onChange={emptyFunction} />,
container,
),
).toWarnDev(
'Warning: A component is changing an uncontrolled input of type checkbox to be controlled. ' +
'Input elements should not switch from uncontrolled to controlled (or vice versa). ' +
Expand Down Expand Up @@ -1183,7 +1262,10 @@ describe('ReactDOMInput', () => {
const container = document.createElement('div');
ReactDOM.render(stub, container);
expect(() =>
ReactDOM.render(<input type="radio" checked={null} />, container),
ReactDOM.render(
<input type="radio" checked={null} onChange={emptyFunction} />,
container,
),
).toWarnDev(
'Warning: A component is changing a controlled input of type radio to be uncontrolled. ' +
'Input elements should not switch from controlled to uncontrolled (or vice versa). ' +
Expand Down Expand Up @@ -1224,11 +1306,14 @@ describe('ReactDOMInput', () => {
});

it('should warn if uncontrolled radio (checked is null) switches to controlled', () => {
const stub = <input type="radio" checked={null} />;
const stub = <input type="radio" checked={null} onChange={emptyFunction} />;
const container = document.createElement('div');
ReactDOM.render(stub, container);
expect(() =>
ReactDOM.render(<input type="radio" checked={true} />, container),
ReactDOM.render(
<input type="radio" checked={true} onChange={emptyFunction} />,
container,
),
).toWarnDev(
'Warning: A component is changing an uncontrolled input of type radio to be controlled. ' +
'Input elements should not switch from uncontrolled to controlled (or vice versa). ' +
Expand Down
10 changes: 6 additions & 4 deletions packages/react-dom/src/__tests__/ReactDOMSelect-test.js
Expand Up @@ -9,6 +9,8 @@

'use strict';

const emptyFunction = require('fbjs/lib/emptyFunction');

describe('ReactDOMSelect', () => {
let React;
let ReactDOM;
Expand Down Expand Up @@ -534,7 +536,7 @@ describe('ReactDOMSelect', () => {
it('should warn if value is null', () => {
expect(() =>
ReactTestUtils.renderIntoDocument(
<select value={null}>
<select value={null} onChange={emptyFunction}>
<option value="test" />
</select>,
),
Expand All @@ -545,7 +547,7 @@ describe('ReactDOMSelect', () => {
);

ReactTestUtils.renderIntoDocument(
<select value={null}>
<select value={null} onChange={emptyFunction}>
<option value="test" />
</select>,
);
Expand Down Expand Up @@ -575,7 +577,7 @@ describe('ReactDOMSelect', () => {
it('should warn if value is null and multiple is true', () => {
expect(() =>
ReactTestUtils.renderIntoDocument(
<select value={null} multiple={true}>
<select value={null} multiple={true} onChange={emptyFunction}>
<option value="test" />
</select>,
),
Expand All @@ -587,7 +589,7 @@ describe('ReactDOMSelect', () => {
);

ReactTestUtils.renderIntoDocument(
<select value={null} multiple={true}>
<select value={null} multiple={true} onChange={emptyFunction}>
<option value="test" />
</select>,
);
Expand Down
10 changes: 7 additions & 3 deletions packages/react-dom/src/__tests__/ReactDOMTextarea-test.js
Expand Up @@ -97,7 +97,7 @@ describe('ReactDOMTextarea', () => {
});

it('should display `value` of number 0', () => {
const stub = <textarea value={0} />;
const stub = <textarea value={0} onChange={emptyFunction} />;
const node = renderTextarea(stub);

expect(node.value).toBe('0');
Expand Down Expand Up @@ -373,15 +373,19 @@ describe('ReactDOMTextarea', () => {

it('should warn if value is null', () => {
expect(() =>
ReactTestUtils.renderIntoDocument(<textarea value={null} />),
ReactTestUtils.renderIntoDocument(
<textarea value={null} onChange={emptyFunction} />,
),
).toWarnDev(
'`value` prop on `textarea` should not be null. ' +
'Consider using an empty string to clear the component or `undefined` ' +
'for uncontrolled components.',
);

// No additional warnings are expected
ReactTestUtils.renderIntoDocument(<textarea value={null} />);
ReactTestUtils.renderIntoDocument(
<textarea value={null} onChange={emptyFunction} />,
);
});

it('should warn if value and defaultValue are specified', () => {
Expand Down
10 changes: 6 additions & 4 deletions packages/react-dom/src/shared/ReactControlledValuePropTypes.js
Expand Up @@ -25,11 +25,12 @@ if (__DEV__) {
const propTypes = {
value: function(props, propName, componentName) {
if (
!props[propName] ||
!(propName in props) ||
hasReadOnlyValue[props.type] ||
props.onChange ||
props.readOnly ||
props.disabled
props.disabled ||
props.value === null
) {
return null;
}
Expand All @@ -42,10 +43,11 @@ if (__DEV__) {
},
checked: function(props, propName, componentName) {
if (
!props[propName] ||
!(propName in props) ||
props.onChange ||
props.readOnly ||
props.disabled
props.disabled ||
props.value === null
) {
return null;
}
Expand Down