@samitouri / QOS-React / commits / e5d85fbe04

Remove pendingUpdate optimization in ReactDOMSelect (#8175)

This removes an optimization that avoids call update on ReactDOMSelect twice in an onChange event. https://github.com/facebook/react/commit/2601b6a0b0c2b0bccaca67fbbfab6987ac5bef30#commitcomment-19643403 None of the other controlled components do this. The only reason to do it here is because of the loop. I'd like to remove this because I'd like to remove all the side-effects that happen in onChange, other than user defined behavior. I'd also like to get rid of state that track sequences. It is easier if everything is just diffing. Alternatively I can store the previous value that we processed and only reprocess if the value has changed. However, that would requires the array for multiple values to be immutable and I don't think we enforce that right now. In Fiber, I believe that we'll be able to batch both these updates into a single commit.

Sebastian Markbåge committed Nov 1, 2016 at 18:54 UTC e5d85fbe04dda86cd71e8a825537a70ec85a82df
2 files changed +9 -19
src/renderers/dom/shared/wrappers/__tests__/ReactDOMSelect-test.js
+1 -3
@@ -497,9 +497,7 @@ describe('ReactDOMSelect', () => {
497 stub = ReactDOM.render(stub, container);
498 var node = ReactDOM.findDOMNode(stub);
499
500 - expect(() => ReactTestUtils.Simulate.change(node)).not.toThrow(
501 - "Cannot set property 'pendingUpdate' of null"
502 - );
500 + expect(() => ReactTestUtils.Simulate.change(node)).not.toThrow();
501 });
502
503 it('should select grandchild options nested inside an optgroup', () => {
src/renderers/dom/stack/client/wrappers/ReactDOMSelect.js
+8 -16
@@ -19,10 +19,8 @@ var warning = require('warning');
19
20 var didWarnValueDefaultValue = false;
21
22 -function updateOptionsIfPendingUpdateAndMounted() {
23 - if (this._rootNodeID && this._wrapperState.pendingUpdate) {
24 - this._wrapperState.pendingUpdate = false;
25 -
22 +function forceUpdateIfMounted() {
23 + if (this._rootNodeID) {
24 var props = this._currentElement.props;
25 var value = props.value;
26
@@ -89,15 +87,14 @@ function checkSelectPropTypes(inst, props) {
87 * @private
88 */
89 function updateOptions(inst, multiple, propValue) {
92 - var selectedValue, i;
90 var options = ReactDOMComponentTree.getNodeFromInstance(inst).options;
91
92 if (multiple) {
96 - selectedValue = {};
97 - for (i = 0; i < propValue.length; i++) {
93 + let selectedValue = {};
94 + for (let i = 0; i < propValue.length; i++) {
95 selectedValue['' + propValue[i]] = true;
96 }
100 - for (i = 0; i < options.length; i++) {
97 + for (let i = 0; i < options.length; i++) {
98 var selected = selectedValue.hasOwnProperty(options[i].value);
99 if (options[i].selected !== selected) {
100 options[i].selected = selected;
@@ -106,8 +103,8 @@ function updateOptions(inst, multiple, propValue) {
103 } else {
104 // Do not set `select.value` as exact behavior isn't consistent across all
105 // browsers for all cases.
109 - selectedValue = '' + propValue;
110 - for (i = 0; i < options.length; i++) {
106 + let selectedValue = '' + propValue;
107 + for (let i = 0; i < options.length; i++) {
108 if (options[i].value === selectedValue) {
109 options[i].selected = true;
110 return;
@@ -149,7 +146,6 @@ var ReactDOMSelect = {
146
147 var value = props.value;
148 inst._wrapperState = {
152 - pendingUpdate: false,
149 initialValue: value != null ? value : props.defaultValue,
150 listeners: null,
151 onChange: _handleChange.bind(inst),
@@ -191,7 +187,6 @@ var ReactDOMSelect = {
187
188 var value = props.value;
189 if (value != null) {
194 - inst._wrapperState.pendingUpdate = false;
190 updateOptions(inst, Boolean(props.multiple), value);
191 } else if (wasMultiple !== Boolean(props.multiple)) {
192 // For simplicity, reapply `defaultValue` if `multiple` is toggled.
@@ -212,10 +207,7 @@ function _handleChange(event) {
207 returnValue = props.onChange.call(undefined, event);
208 }
209
215 - if (this._rootNodeID) {
216 - this._wrapperState.pendingUpdate = true;
217 - }
218 - ReactUpdates.asap(updateOptionsIfPendingUpdateAndMounted, this);
210 + ReactUpdates.asap(forceUpdateIfMounted, this);
211 return returnValue;
212 }
213