Skip to content

Commit

Permalink
Issue: 5013 Added necessary code for firing warning if value is null
Browse files Browse the repository at this point in the history
Fixed the lint issues

Added logic for handling the warning only once and added the test cases for the same. Also moved the warning part to only DEV mode

Changed few lines related to the formatting issues

Removing the empty whitespace
  • Loading branch information
antsmartian committed Oct 15, 2015
1 parent da11356 commit b42c1da
Show file tree
Hide file tree
Showing 6 changed files with 94 additions and 1 deletion.
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.'
);

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.'
);

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);
});
});

0 comments on commit b42c1da

Please sign in to comment.