Skip to content

Commit

Permalink
Fix “no onChange handler” warning to fire on falsy values ("", 0, fal…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
wherestheguac authored and gaearon committed Jul 17, 2018
1 parent 606c30a commit 171e0b7
Show file tree
Hide file tree
Showing 5 changed files with 128 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,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={() => {}} />, container);
expect(container.firstChild.getAttribute('value')).toBe('');
expect(container.firstChild.value).toBe('');
});
Expand Down
136 changes: 106 additions & 30 deletions packages/react-dom/src/__tests__/ReactDOMInput-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,71 @@ describe('ReactDOMInput', () => {
document.body.removeChild(container);
});

it('should warn for controlled value of 0 with missing onChange', () => {
expect(() => {
ReactDOM.render(<input type="text" value={0} />, container);
}).toWarnDev(
'Failed prop type: You provided a `value` prop to a form field without an `onChange` handler.',
);
});

it('should warn for controlled value of "" with missing onChange', () => {
expect(() => {
ReactDOM.render(<input type="text" value="" />, container);
}).toWarnDev(
'Failed prop type: You provided a `value` prop to a form field without an `onChange` handler.',
);
});

it('should warn for controlled value of "0" with missing onChange', () => {
expect(() => {
ReactDOM.render(<input type="text" value="0" />, container);
}).toWarnDev(
'Failed prop type: You provided a `value` prop to a form field without an `onChange` handler.',
);
});

it('should warn for controlled value of false with missing onChange', () => {
expect(() =>
ReactDOM.render(<input type="checkbox" checked={false} />, container),
).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', () => {
ReactDOM.render(
<input type="checkbox" checked={false} readOnly={true} />,
container,
);
ReactDOM.unmountComponentAtNode(container);

expect(() =>
ReactDOM.render(
<input type="checkbox" checked={false} readOnly={false} />,
container,
),
).toWarnDev(
'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`.',
);
});

it('should not warn about missing onChange in uncontrolled inputs', () => {
ReactDOM.render(<input />, container);
ReactDOM.unmountComponentAtNode(container);
ReactDOM.render(<input value={undefined} />, container);
ReactDOM.unmountComponentAtNode(container);
ReactDOM.render(<input type="text" />, container);
ReactDOM.unmountComponentAtNode(container);
ReactDOM.render(<input type="text" value={undefined} />, container);
ReactDOM.unmountComponentAtNode(container);
ReactDOM.render(<input type="checkbox" />, container);
ReactDOM.unmountComponentAtNode(container);
ReactDOM.render(<input type="checkbox" checked={undefined} />, container);
});

it('should properly control a value even if no event listener exists', () => {
let node;

Expand Down Expand Up @@ -452,7 +517,7 @@ describe('ReactDOMInput', () => {
});

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

expect(node.value).toBe('0');
Expand Down Expand Up @@ -590,8 +655,14 @@ describe('ReactDOMInput', () => {
});

it('should properly transition from an empty value to 0', function() {
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 @@ -600,8 +671,14 @@ describe('ReactDOMInput', () => {
});

it('should properly transition from 0 to an empty value', function() {
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 @@ -610,8 +687,14 @@ describe('ReactDOMInput', () => {
});

it('should properly transition a text input from 0 to an empty 0.0', function() {
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 @@ -620,8 +703,14 @@ describe('ReactDOMInput', () => {
});

it('should properly transition a number input from "" to 0', function() {
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 @@ -630,8 +719,14 @@ describe('ReactDOMInput', () => {
});

it('should properly transition a number input from "" to "0"', function() {
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 @@ -874,25 +969,6 @@ describe('ReactDOMInput', () => {
dispatchEventOnNode(node, 'input');
});

it('should warn with checked and no onChange handler with readOnly specified', () => {
ReactDOM.render(
<input type="checkbox" checked="false" readOnly={true} />,
container,
);
ReactDOM.unmountComponentAtNode(container);

expect(() =>
ReactDOM.render(
<input type="checkbox" checked="false" readOnly={false} />,
container,
),
).toWarnDev(
'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`.',
);
});

it('should update defaultValue to empty string', () => {
ReactDOM.render(<input type="text" defaultValue={'foo'} />, container);
ReactDOM.render(<input type="text" defaultValue={''} />, container);
Expand Down
7 changes: 7 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMSelect-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,13 @@ describe('ReactDOMSelect', () => {
);
});

it('should not warn about missing onChange in uncontrolled textareas', () => {
const container = document.createElement('div');
ReactDOM.render(<select />, container);
ReactDOM.unmountComponentAtNode(container);
ReactDOM.render(<select value={undefined} />, container);
});

it('should be able to safely remove select onChange', () => {
function changeView() {
ReactDOM.unmountComponentAtNode(container);
Expand Down
9 changes: 8 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMTextarea-test.js
Original file line number Diff line number Diff line change
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 @@ -416,4 +416,11 @@ describe('ReactDOMTextarea', () => {
<textarea value="foo" defaultValue="bar" readOnly={true} />,
);
});

it('should not warn about missing onChange in uncontrolled textareas', () => {
const container = document.createElement('div');
ReactDOM.render(<textarea />, container);
ReactDOM.unmountComponentAtNode(container);
ReactDOM.render(<textarea value={undefined} />, container);
});
});
10 changes: 6 additions & 4 deletions packages/react-dom/src/shared/ReactControlledValuePropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,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[propName] == null
) {
return null;
}
Expand All @@ -47,10 +48,11 @@ if (__DEV__) {
},
checked: function(props, propName, componentName) {
if (
!props[propName] ||
!(propName in props) ||
props.onChange ||
props.readOnly ||
props.disabled
props.disabled ||
props[propName] == null
) {
return null;
}
Expand Down

0 comments on commit 171e0b7

Please sign in to comment.