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

Issue: 5013 Added necessary code for firing warning if value is null #5048

Merged
merged 1 commit into from
Oct 15, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 19 additions & 0 deletions src/renderers/dom/client/wrappers/ReactDOMInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var instancesByReactID = {};

var didWarnValueLink = false;
var didWarnCheckedLink = false;
var didWarnValueNull = false;

function forceUpdateIfMounted() {
if (this._rootNodeID) {
Expand All @@ -32,6 +33,19 @@ function forceUpdateIfMounted() {
}
}

function warnIfValueIsNull(props) {
if (props != null && props.value === null && !didWarnValueNull) {
warning(
false,
'`value` prop on `input` should not be null. ' +
'Consider using the empty string to clear the component or' +
' `undefined` for uncontrolled components.'
);

didWarnValueNull = true;
}
}

/**
* Implements an <input> native component that allows setting these optional
* props: `checked`, `value`, `defaultChecked`, and `defaultValue`.
Expand Down Expand Up @@ -86,6 +100,7 @@ var ReactDOMInput = {
);
didWarnCheckedLink = true;
}
warnIfValueIsNull(props);
}

var defaultValue = props.defaultValue;
Expand All @@ -108,6 +123,10 @@ var ReactDOMInput = {
updateWrapper: function(inst) {
var props = inst._currentElement.props;

if (__DEV__) {
warnIfValueIsNull(props);
}

// TODO: Shouldn't this be getChecked(props)?
var checked = props.checked;
if (checked != null) {
Expand Down
18 changes: 18 additions & 0 deletions src/renderers/dom/client/wrappers/ReactDOMSelect.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ var assign = require('Object.assign');
var warning = require('warning');

var didWarnValueLink = false;
var didWarnValueNull = false;

function updateOptionsIfPendingUpdateAndMounted() {
if (this._rootNodeID && this._wrapperState.pendingUpdate) {
Expand All @@ -43,6 +44,19 @@ function getDeclarationErrorAddendum(owner) {
return '';
}

function warnIfValueIsNull(props) {
if (props != null && props.value === null && !didWarnValueNull) {
warning(
false,
'`value` prop on `select` should not be null. ' +
'Consider using the empty string to clear the component or' +
' `undefined` for uncontrolled components.'
);

didWarnValueNull = true;
}
}

var valuePropNames = ['value', 'defaultValue'];

/**
Expand Down Expand Up @@ -153,6 +167,7 @@ var ReactDOMSelect = {
mountWrapper: function(inst, props) {
if (__DEV__) {
checkSelectPropTypes(inst, props);
warnIfValueIsNull(props);
}

var value = LinkedValueUtils.getValue(props);
Expand All @@ -172,6 +187,9 @@ var ReactDOMSelect = {

postUpdateWrapper: function(inst) {
var props = inst._currentElement.props;
if (__DEV__) {
warnIfValueIsNull(props);
}

// After the initial mount, we control selected-ness manually so don't pass
// this value down
Expand Down
21 changes: 20 additions & 1 deletion src/renderers/dom/client/wrappers/ReactDOMTextarea.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ var invariant = require('invariant');
var warning = require('warning');

var didWarnValueLink = false;
var didWarnValueNull = false;

function forceUpdateIfMounted() {
if (this._rootNodeID) {
Expand All @@ -28,6 +29,19 @@ function forceUpdateIfMounted() {
}
}

function warnIfValueIsNull(props) {
if (props != null && props.value === null && !didWarnValueNull) {
warning(
false,
'`value` prop on `textarea` should not be null. ' +
'Consider using the empty string to clear the component or' +
' `undefined` for uncontrolled components.'
);

didWarnValueNull = true;
}
}

/**
* Implements a <textarea> native component that allows setting `value`, and
* `defaultValue`. This differs from the traditional DOM API because value is
Expand Down Expand Up @@ -76,6 +90,7 @@ var ReactDOMTextarea = {
);
didWarnValueLink = true;
}
warnIfValueIsNull(props);
}

var defaultValue = props.defaultValue;
Expand Down Expand Up @@ -107,7 +122,6 @@ var ReactDOMTextarea = {
defaultValue = '';
}
var value = LinkedValueUtils.getValue(props);

inst._wrapperState = {
// We save the initial value so that `ReactDOMComponent` doesn't update
// `textContent` (unnecessary since we update value).
Expand All @@ -120,6 +134,11 @@ var ReactDOMTextarea = {

updateWrapper: function(inst) {
var props = inst._currentElement.props;

if (__DEV__) {
warnIfValueIsNull(props);
}

var value = LinkedValueUtils.getValue(props);
if (value != null) {
// Cast `value` to a string to ensure the value is set correctly. While
Expand Down
11 changes: 11 additions & 0 deletions src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -403,4 +403,15 @@ describe('ReactDOMInput', function() {
<input type="checkbox" checkedLink={link} valueLink={emptyFunction} />;
expect(() => ReactDOM.render(instance, node)).toThrow();
});

it('should throw warning message if value is null', function() {
ReactTestUtils.renderIntoDocument(<input type="text" value={null} />);
expect(console.error.argsForCall[0][0]).toContain(
'`value` prop on `input` should not be null. Consider using the empty string to clear the component or'
+ ' `undefined` for uncontrolled components.'
Copy link
Contributor

Choose a reason for hiding this comment

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

" or "

);

ReactTestUtils.renderIntoDocument(<input type="text" value={null} />);
expect(console.error.argsForCall.length).toBe(1);
});
});
13 changes: 13 additions & 0 deletions src/renderers/dom/client/wrappers/__tests__/ReactDOMSelect-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -463,4 +463,17 @@ describe('ReactDOMSelect', function() {
expect(node.options[1].selected).toBe(false); // giraffe
expect(node.options[2].selected).toBe(false); // gorilla
});

it('should throw warning message if value is null', function() {
spyOn(console, 'error');

ReactTestUtils.renderIntoDocument(<select value={null}><option value="test"/></select>);
expect(console.error.argsForCall[0][0]).toContain(
'`value` prop on `select` should not be null. Consider using the empty string to clear the component or'
+ ' `undefined` for uncontrolled components.'
);

Copy link
Contributor

Choose a reason for hiding this comment

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

We should assert that the console.error contains the expected error message. See how I did this in https://github.com/facebook/react/pull/5032/files

ReactTestUtils.renderIntoDocument(<select value={null}><option value="test"/></select>);
expect(console.error.argsForCall.length).toBe(1);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -265,4 +265,17 @@ describe('ReactDOMTextarea', function() {
renderTextarea(<textarea />, container);
ReactDOM.unmountComponentAtNode(container);
});

it('should throw warning message if value is null', function() {
spyOn(console, 'error');

ReactTestUtils.renderIntoDocument(<textarea value={null} />);
expect(console.error.argsForCall[0][0]).toContain(
'`value` prop on `textarea` should not be null. Consider using the empty string to clear the component or'
+ ' `undefined` for uncontrolled components.'
);

ReactTestUtils.renderIntoDocument(<textarea value={null} />);
expect(console.error.argsForCall.length).toBe(1);
});
});