Skip to content

Changed the error message displayed when a select element has props.multiple set to true and value set to null #11141

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 31, 2017
Merged

Changed the error message displayed when a select element has props.multiple set to true and value set to null #11141

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
908ad12
Corrects error message for select with props.multiple set to true and…
nealwright Oct 7, 2017
0b2eefd
Don't bother deduplicating based on type
gaearon Oct 31, 2017
182e42f
Make the code a bit simpler (and more verbose)
gaearon Oct 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-dom/src/__tests__/ReactDOMInput-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -796,7 +796,7 @@ describe('ReactDOMInput', () => {
ReactTestUtils.renderIntoDocument(<input type="text" value={null} />);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'`value` prop on `input` should not be null. ' +
'Consider using the empty string to clear the component or `undefined` ' +
'Consider using an empty string to clear the component or `undefined` ' +
'for uncontrolled components.',
);

Expand Down
21 changes: 20 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMSelect-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ describe('ReactDOMSelect', () => {
);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'`value` prop on `select` should not be null. ' +
'Consider using the empty string to clear the component or `undefined` ' +
'Consider using an empty string to clear the component or `undefined` ' +
'for uncontrolled components.',
);

Expand All @@ -524,6 +524,25 @@ describe('ReactDOMSelect', () => {
expectDev(console.error.calls.count()).toBe(1);
});

it('should warn if value is null and multiple is true', () => {
spyOn(console, 'error');
ReactTestUtils.renderIntoDocument(
<select value={null} multiple={true}><option value="test" /></select>,
);

expectDev(console.error.calls.argsFor(0)[0]).toContain(
'`value` prop on `select` should not be null. ' +
'Consider using an empty array when `multiple` is ' +
'set to `true` to clear the component or `undefined` ' +
'for uncontrolled components.',
);

ReactTestUtils.renderIntoDocument(
<select value={null} multiple={true}><option value="test" /></select>,
);
expectDev(console.error.calls.count()).toBe(1);
});

it('should refresh state on change', () => {
var stub = (
<select value="giraffe" onChange={noop}>
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/__tests__/ReactDOMTextarea-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ describe('ReactDOMTextarea', () => {
ReactTestUtils.renderIntoDocument(<textarea value={null} />);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'`value` prop on `textarea` should not be null. ' +
'Consider using the empty string to clear the component or `undefined` ' +
'Consider using an empty string to clear the component or `undefined` ' +
'for uncontrolled components.',
);

Expand Down
19 changes: 15 additions & 4 deletions packages/react-dom/src/shared/ReactDOMNullInputValuePropHook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ if (__DEV__) {
var warning = require('fbjs/lib/warning');
}

var didWarnValueNull = false;
var didWarnValueNull = {
default: false,
multiple: false,
};

function getStackAddendum() {
var stack = ReactDebugCurrentFrame.getStackAddendum();
Expand All @@ -23,17 +26,25 @@ function validateProperties(type, props) {
if (type !== 'input' && type !== 'textarea' && type !== 'select') {
return;
}
if (props != null && props.value === null && !didWarnValueNull) {

var isMultipleSelect = type === 'select' && props.multiple === true;
var errorType = isMultipleSelect ? 'multiple' : 'default';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is important. I'd prefer a single boolean to deduplicate (even though deduplicating one will influence the other). They're rare enough.

Then we would move the isMultipleSelect check inside the condition. So it's only checked in error case.

var isFirstError = !didWarnValueNull[errorType];

if (props != null && props.value === null && isFirstError) {
warning(
false,
'`value` prop on `%s` should not be null. ' +
'Consider using the empty string to clear the component or `undefined` ' +
'Consider using an empty %s to clear the component or `undefined` ' +
'for uncontrolled components.%s',
type,
errorType === 'multiple'
? 'array when `multiple` is set to `true`'
: 'string',
getStackAddendum(),
);

didWarnValueNull = true;
didWarnValueNull[errorType] = true;
}
}

Expand Down