Skip to content

Commit

Permalink
Inputs should not mutate value on type conversion (#9806)
Browse files Browse the repository at this point in the history
This is a follow-up on
#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 aweary committed May 30, 2017
1 parent 80bdebc commit 74044e0
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 2 deletions.
2 changes: 2 additions & 0 deletions scripts/fiber/tests-passing.txt
Expand Up @@ -1536,6 +1536,8 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
* should allow setting `value` to `false`
* should allow setting `value` to `objToString`
* should not incur unnecessary DOM mutations
* should not incur unnecessary DOM mutations for numeric type conversion
* should not incur unnecessary DOM mutations for the boolean type conversion
* should properly control a value of number `0`
* should properly control 0.0 for a text input
* should properly control 0.0 for a number input
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js
Expand Up @@ -209,7 +209,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/shared/wrappers/__tests__/ReactDOMInput-test.js
Expand Up @@ -471,6 +471,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
2 changes: 1 addition & 1 deletion src/renderers/dom/stack/client/wrappers/ReactDOMInput.js
Expand Up @@ -201,7 +201,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

0 comments on commit 74044e0

Please sign in to comment.