From 74044e0ebacd9510e78e9a3879aa65e724dce4a3 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Tue, 30 May 2017 16:25:44 -0400 Subject: [PATCH] Inputs should not mutate value on type conversion (#9806) This is a follow-up on https://github.com/facebook/react/pull/9584#discussion_r115642293. 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(, el) DOM.render(, 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; ``` --- scripts/fiber/tests-passing.txt | 2 + .../dom/fiber/wrappers/ReactDOMFiberInput.js | 2 +- .../wrappers/__tests__/ReactDOMInput-test.js | 40 +++++++++++++++++++ .../stack/client/wrappers/ReactDOMInput.js | 2 +- 4 files changed, 44 insertions(+), 2 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 111ea1692e20..c0a53983ff00 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -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 diff --git a/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js b/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js index 41de9687e429..6d5387e25660 100644 --- a/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js +++ b/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js @@ -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; diff --git a/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js b/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js index 19a38b9e182f..6c7df548136e 100644 --- a/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js +++ b/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js @@ -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(, 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(, 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(, 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(, container); + expect(nodeValueSetter.mock.calls.length).toBe(0); + }); + it('should properly control a value of number `0`', () => { var stub = ; stub = ReactTestUtils.renderIntoDocument(stub); diff --git a/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js b/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js index b05ca82b352d..a993df0543b7 100644 --- a/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js +++ b/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js @@ -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;