Skip to content

Commit

Permalink
Inputs should not mutate value on type conversion (facebook#9806)
Browse files Browse the repository at this point in the history
This is a follow-up on
facebook#9584 (comment). There
is no need to assign the value property of an input if the value
property of the React component changes types, but stringifies to the
same value. For example:

```javascript
DOM.render(<input value="true" />, el)
DOM.render(<input value={true} />, el)
```

In this case, the assignment to `input.value` will always be
cast to the string "true". There is no need to perform this
assignment. Particularly when we already cast the value to a string
later:

```javascript
// Cast `value` to a string to ensure the value is set correctly. While
// browsers typically do this as necessary, jsdom doesn't.
node.value = '' + value;
```
  • Loading branch information
nhunzaker authored and flarnie committed Jun 7, 2017
1 parent b4c541e commit a17dc9c
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/renderers/dom/client/wrappers/ReactDOMInput.js
Expand Up @@ -226,7 +226,7 @@ var ReactDOMInput = {
// browsers typically do this as necessary, jsdom doesn't.
node.value = '' + value;
}
} else if (node.value !== value) {
} else if (node.value !== '' + value) {
// Cast `value` to a string to ensure the value is set correctly. While
// browsers typically do this as necessary, jsdom doesn't.
node.value = '' + value;
Expand Down
40 changes: 40 additions & 0 deletions src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js
Expand Up @@ -324,6 +324,46 @@ describe('ReactDOMInput', () => {
expect(nodeValueSetter.mock.calls.length).toBe(1);
});

it('should not incur unnecessary DOM mutations for numeric type conversion', () => {
var container = document.createElement('div');
ReactDOM.render(<input value="0" />, container);

var node = container.firstChild;
var nodeValue = '0';
var nodeValueSetter = jest.genMockFn();
Object.defineProperty(node, 'value', {
get: function() {
return nodeValue;
},
set: nodeValueSetter.mockImplementation(function(newValue) {
nodeValue = newValue;
}),
});

ReactDOM.render(<input value={0} />, container);
expect(nodeValueSetter.mock.calls.length).toBe(0);
});

it('should not incur unnecessary DOM mutations for the boolean type conversion', () => {
var container = document.createElement('div');
ReactDOM.render(<input value="true" />, container);

var node = container.firstChild;
var nodeValue = 'true';
var nodeValueSetter = jest.genMockFn();
Object.defineProperty(node, 'value', {
get: function() {
return nodeValue;
},
set: nodeValueSetter.mockImplementation(function(newValue) {
nodeValue = newValue;
}),
});

ReactDOM.render(<input value={true} />, container);
expect(nodeValueSetter.mock.calls.length).toBe(0);
});

it('should properly control a value of number `0`', () => {
var stub = <input type="text" value={0} onChange={emptyFunction} />;
stub = ReactTestUtils.renderIntoDocument(stub);
Expand Down

0 comments on commit a17dc9c

Please sign in to comment.