Use explicit pass for restoring controlled state instead of asap (#8176)
* Use explicit pass for restoring controlled state instead of asap This gets rid of the first class ReactUpdates.asap scheduling. Instead of scheduling a first class function to invoke later, I introduce an explicit phase for restoring controlled state after dispatching events. We assume that the deepest event target is the thing that needs its state restored. * Drop special cases for onChange in wrappers We're now not using anything special about onChange handling in the wrappers. Yay!
Sebastian Markbåge committed
Nov 7, 2016 at 13:31 UTC
90878df08d4984243096de6a9c79c6bd11f43774
13 files changed
+143
-187
scripts/fiber/tests-failing.txt
-2
@@ -576,8 +576,6 @@ src/renderers/shared/stack/reconciler/__tests__/ReactUpdates-test.js
576
* should queue mount-ready handlers across different roots
577
* should flush updates in the correct order across roots
578
* should queue updates from during mount
579
-* calls asap callbacks properly
580
-* calls asap callbacks with queued updates
579
* does not call render after a component as been deleted
580
* marks top-level updates
581
* throws in setState if the update callback is not a function
src/renderers/dom/shared/ReactEventListener.js
+21
-7
@@ -14,6 +14,7 @@
14
var EventListener = require('EventListener');
15
var ExecutionEnvironment = require('ExecutionEnvironment');
16
var PooledClass = require('PooledClass');
17
+var ReactControlledComponent = require('ReactControlledComponent');
18
var ReactDOMComponentTree = require('ReactDOMComponentTree');
19
var ReactGenericBatching = require('ReactGenericBatching');
20
@@ -38,28 +39,27 @@ function findParent(inst) {
39
}
40
41
// Used to store ancestor hierarchy in top level callback
41
-function TopLevelCallbackBookKeeping(topLevelType, nativeEvent) {
42
+function TopLevelCallbackBookKeeping(topLevelType, nativeEvent, targetInst) {
43
this.topLevelType = topLevelType;
44
this.nativeEvent = nativeEvent;
45
+ this.targetInst = targetInst;
46
this.ancestors = [];
47
}
48
Object.assign(TopLevelCallbackBookKeeping.prototype, {
49
destructor: function() {
50
this.topLevelType = null;
51
this.nativeEvent = null;
52
+ this.targetInst = null;
53
this.ancestors.length = 0;
54
},
55
});
56
PooledClass.addPoolingTo(
57
TopLevelCallbackBookKeeping,
55
- PooledClass.twoArgumentPooler
58
+ PooledClass.threeArgumentPooler
59
);
60
61
function handleTopLevelImpl(bookKeeping) {
59
- var nativeEventTarget = getEventTarget(bookKeeping.nativeEvent);
60
- var targetInst = ReactDOMComponentTree.getClosestInstanceFromNode(
61
- nativeEventTarget
62
- );
62
+ var targetInst = bookKeeping.targetInst;
63
64
// Loop through the hierarchy, in case there's any nested components.
65
// It's important that we build the array of ancestors before calling any
@@ -158,14 +158,28 @@ var ReactEventListener = {
158
return;
159
}
160
161
+ var nativeEventTarget = getEventTarget(nativeEvent);
162
+ var targetInst = ReactDOMComponentTree.getClosestInstanceFromNode(
163
+ nativeEventTarget
164
+ );
165
+
166
var bookKeeping = TopLevelCallbackBookKeeping.getPooled(
167
topLevelType,
163
- nativeEvent
168
+ nativeEvent,
169
+ targetInst
170
);
171
+
172
try {
173
// Event queue being processed in the same cycle allows
174
// `preventDefault`.
175
ReactGenericBatching.batchedUpdates(handleTopLevelImpl, bookKeeping);
176
+ if (bookKeeping.targetInst) {
177
+ // Here we wait until all updates have propagated, which is important
178
+ // when using controlled components within layers:
179
+ // https://github.com/facebook/react/issues/1698
180
+ // Then we restore state of any controlled component.
181
+ ReactControlledComponent.restoreStateIfNeeded(bookKeeping.targetInst);
182
+ }
183
} finally {
184
TopLevelCallbackBookKeeping.release(bookKeeping);
185
}
src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js
+6
@@ -14,6 +14,7 @@
14
var EventPluginHub = require('EventPluginHub');
15
var EventPropagators = require('EventPropagators');
16
var ExecutionEnvironment = require('ExecutionEnvironment');
17
+var ReactControlledComponent = require('ReactControlledComponent');
18
var ReactDOMComponentTree = require('ReactDOMComponentTree');
19
var ReactGenericBatching = require('ReactGenericBatching');
20
var SyntheticEvent = require('SyntheticEvent');
@@ -50,6 +51,8 @@ function createAndAccumulateChangeEvent(inst, nativeEvent, target) {
51
target
52
);
53
event.type = 'change';
54
+ // Flag this event loop as needing state restore.
55
+ ReactControlledComponent.enqueueStateRestore();
56
EventPropagators.accumulateTwoPhaseDispatches(event);
57
return event;
58
}
@@ -99,6 +102,9 @@ function manualDispatchChangeEvent(nativeEvent) {
102
// the component is rerendered and the value reverted before all the event
103
// handlers can run. See https://github.com/facebook/react/issues/708.
104
ReactGenericBatching.batchedUpdates(runEventInBatch, event);
105
+ if (activeElementInst) {
106
+ ReactControlledComponent.restoreStateIfNeeded(activeElementInst);
107
+ }
108
}
109
110
function runEventInBatch(event) {
src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
+1
-1
@@ -404,7 +404,7 @@ describe('ReactDOMInput', () => {
404
handleChange = () => {
405
this.setState({
406
changed: true,
407
- })
407
+ });
408
}
409
componentDidUpdate() {
410
sharedParent.appendChild(container2);
src/renderers/dom/stack/client/ReactDOMComponent.js
+14
@@ -1246,6 +1246,20 @@ ReactDOMComponent.Mixin = {
1246
}
1247
},
1248
1249
+ restoreControlledState: function() {
1250
+ switch (this._tag) {
1251
+ case 'input':
1252
+ ReactDOMInput.restoreControlledState(this);
1253
+ return;
1254
+ case 'textarea':
1255
+ ReactDOMTextarea.restoreControlledState(this);
1256
+ return;
1257
+ case 'select':
1258
+ ReactDOMSelect.restoreControlledState(this);
1259
+ return;
1260
+ }
1261
+ },
1262
+
1263
getPublicInstance: function() {
1264
return getNode(this);
1265
},
src/renderers/dom/stack/client/wrappers/ReactDOMInput.js
+9
-28
@@ -14,7 +14,6 @@
14
var DOMPropertyOperations = require('DOMPropertyOperations');
15
var ReactControlledValuePropTypes = require('ReactControlledValuePropTypes');
16
var ReactDOMComponentTree = require('ReactDOMComponentTree');
17
-var ReactUpdates = require('ReactUpdates');
17
18
var invariant = require('invariant');
19
var warning = require('warning');
@@ -24,15 +23,6 @@ var didWarnCheckedDefaultChecked = false;
23
var didWarnControlledToUncontrolled = false;
24
var didWarnUncontrolledToControlled = false;
25
27
-function forceUpdateIfMounted() {
28
- if (this._rootNodeID) {
29
- // DOM component is still mounted; update
30
- ReactDOMInput.updateWrapper(this);
31
- }
32
- var props = this._currentElement.props;
33
- updateNamedCousins(this, props);
34
-}
35
-
26
function isControlled(props) {
27
var usesChecked = props.type === 'checkbox' || props.type === 'radio';
28
return usesChecked ? props.checked != null : props.value != null;
@@ -75,7 +65,6 @@ var ReactDOMInput = {
65
defaultValue: undefined,
66
value: value != null ? value : inst._wrapperState.initialValue,
67
checked: checked != null ? checked : inst._wrapperState.initialChecked,
78
- onChange: inst._wrapperState.onChange,
68
});
69
70
return hostProps;
@@ -134,7 +123,6 @@ var ReactDOMInput = {
123
initialChecked: props.checked != null ? props.checked : props.defaultChecked,
124
initialValue: props.value != null ? props.value : defaultValue,
125
listeners: null,
137
- onChange: _handleChange.bind(inst),
126
};
127
128
if (__DEV__) {
@@ -265,23 +253,16 @@ var ReactDOMInput = {
253
node.name = name;
254
}
255
},
268
-};
269
-
270
-function _handleChange(event) {
271
- var props = this._currentElement.props;
272
-
273
- var returnValue;
274
- if (props.onChange) {
275
- returnValue = props.onChange.call(undefined, event);
276
- }
256
278
- // Here we use asap to wait until all updates have propagated, which
279
- // is important when using controlled components within layers:
280
- // https://github.com/facebook/react/issues/1698
281
- ReactUpdates.asap(forceUpdateIfMounted, this);
282
-
283
- return returnValue;
284
-}
257
+ restoreControlledState: function(inst) {
258
+ if (inst._rootNodeID) {
259
+ // DOM component is still mounted; update
260
+ ReactDOMInput.updateWrapper(inst);
261
+ }
262
+ var props = inst._currentElement.props;
263
+ updateNamedCousins(inst, props);
264
+ },
265
+};
266
267
function updateNamedCousins(thisInstance, props) {
268
var name = props.name;
src/renderers/dom/stack/client/wrappers/ReactDOMSelect.js
+10
-24
@@ -13,23 +13,11 @@
13
14
var ReactControlledValuePropTypes = require('ReactControlledValuePropTypes');
15
var ReactDOMComponentTree = require('ReactDOMComponentTree');
16
-var ReactUpdates = require('ReactUpdates');
16
17
var warning = require('warning');
18
19
var didWarnValueDefaultValue = false;
20
22
-function forceUpdateIfMounted() {
23
- if (this._rootNodeID) {
24
- var props = this._currentElement.props;
25
- var value = props.value;
26
-
27
- if (value != null) {
28
- updateOptions(this, Boolean(props.multiple), value);
29
- }
30
- }
31
-}
32
-
21
function getDeclarationErrorAddendum(owner) {
22
if (owner) {
23
var name = owner.getName();
@@ -134,7 +122,6 @@ function updateOptions(inst, multiple, propValue) {
122
var ReactDOMSelect = {
123
getHostProps: function(inst, props) {
124
return Object.assign({}, props, {
137
- onChange: inst._wrapperState.onChange,
125
value: undefined,
126
});
127
},
@@ -148,7 +135,6 @@ var ReactDOMSelect = {
135
inst._wrapperState = {
136
initialValue: value != null ? value : props.defaultValue,
137
listeners: null,
151
- onChange: _handleChange.bind(inst),
138
wasMultiple: Boolean(props.multiple),
139
};
140
@@ -198,17 +184,17 @@ var ReactDOMSelect = {
184
}
185
}
186
},
201
-};
187
203
-function _handleChange(event) {
204
- var props = this._currentElement.props;
205
- var returnValue;
206
- if (props.onChange) {
207
- returnValue = props.onChange.call(undefined, event);
208
- }
188
+ restoreControlledState: function(inst) {
189
+ if (inst._rootNodeID) {
190
+ var props = inst._currentElement.props;
191
+ var value = props.value;
192
210
- ReactUpdates.asap(forceUpdateIfMounted, this);
211
- return returnValue;
212
-}
193
+ if (value != null) {
194
+ updateOptions(inst, Boolean(props.multiple), value);
195
+ }
196
+ }
197
+ },
198
+};
199
200
module.exports = ReactDOMSelect;
src/renderers/dom/stack/client/wrappers/ReactDOMTextarea.js
+8
-20
@@ -13,20 +13,12 @@
13
14
var ReactControlledValuePropTypes = require('ReactControlledValuePropTypes');
15
var ReactDOMComponentTree = require('ReactDOMComponentTree');
16
-var ReactUpdates = require('ReactUpdates');
16
17
var invariant = require('invariant');
18
var warning = require('warning');
19
20
var didWarnValDefaultVal = false;
21
23
-function forceUpdateIfMounted() {
24
- if (this._rootNodeID) {
25
- // DOM component is still mounted; update
26
- ReactDOMTextarea.updateWrapper(this);
27
- }
28
-}
29
-
22
/**
23
* Implements a <textarea> host component that allows setting `value`, and
24
* `defaultValue`. This differs from the traditional DOM API because value is
@@ -58,7 +50,6 @@ var ReactDOMTextarea = {
50
value: undefined,
51
defaultValue: undefined,
52
children: '' + inst._wrapperState.initialValue,
61
- onChange: inst._wrapperState.onChange,
53
});
54
55
return hostProps;
@@ -128,7 +119,6 @@ var ReactDOMTextarea = {
119
inst._wrapperState = {
120
initialValue: '' + initialValue,
121
listeners: null,
131
- onChange: _handleChange.bind(inst),
122
};
123
},
124
@@ -169,16 +159,14 @@ var ReactDOMTextarea = {
159
node.value = textContent;
160
}
161
},
172
-};
162
174
-function _handleChange(event) {
175
- var props = this._currentElement.props;
176
- var returnValue;
177
- if (props.onChange) {
178
- returnValue = props.onChange.call(undefined, event);
179
- }
180
- ReactUpdates.asap(forceUpdateIfMounted, this);
181
- return returnValue;
182
-}
163
+ restoreControlledState: function(inst) {
164
+ if (inst._rootNodeID) {
165
+ // DOM component is still mounted; update
166
+ ReactDOMTextarea.updateWrapper(inst);
167
+ }
168
+ },
169
+
170
+};
171
172
module.exports = ReactDOMTextarea;
src/renderers/native/ReactNativeEventEmitter.js
+2
@@ -131,6 +131,8 @@ var ReactNativeEventEmitter = {
131
nativeEvent.target
132
);
133
});
134
+ // React Native doesn't use ReactControlledComponent but if it did, here's
135
+ // where it would do it.
136
},
137
138
/**
src/renderers/shared/shared/event/ReactControlledComponent.js
new
+60
@@ -0,0 +1,60 @@
1
+/**
2
+ * Copyright 2013-present, Facebook, Inc.
3
+ * All rights reserved.
4
+ *
5
+ * This source code is licensed under the BSD-style license found in the
6
+ * LICENSE file in the root directory of this source tree. An additional grant
7
+ * of patent rights can be found in the PATENTS file in the same directory.
8
+ *
9
+ * @providesModule ReactControlledComponent
10
+ */
11
+
12
+'use strict';
13
+
14
+var invariant = require('invariant');
15
+
16
+// Use to restore controlled state after a change event has fired.
17
+
18
+var fiberHostComponent = null;
19
+
20
+var ReactControlledComponentInjection = {
21
+ injectFiberControlledHostComponent: function(hostComponentImpl) {
22
+ // The fiber implementation doesn't use dynamic dispatch so we need to
23
+ // inject the implementation.
24
+ fiberHostComponent = hostComponentImpl;
25
+ },
26
+};
27
+
28
+var needsRestoreState = false;
29
+
30
+var ReactControlledComponent = {
31
+ injection: ReactControlledComponentInjection,
32
+
33
+ enqueueStateRestore() {
34
+ needsRestoreState = true;
35
+ },
36
+
37
+ restoreStateIfNeeded(internalInstance) {
38
+ if (!needsRestoreState) {
39
+ return;
40
+ }
41
+ needsRestoreState = false;
42
+ if (typeof internalInstance.tag === 'number') {
43
+ invariant(
44
+ fiberHostComponent &&
45
+ typeof fiberHostComponent.restoreControlledState === 'function',
46
+ 'Fiber needs to be injected to handle a fiber target for controlled ' +
47
+ 'events.'
48
+ );
49
+ fiberHostComponent.restoreControlledState(internalInstance);
50
+ }
51
+ invariant(
52
+ typeof internalInstance.restoreControlledState === 'function',
53
+ 'The internal instance must be a React host component.'
54
+ );
55
+ // If it is not a Fiber, we can just use dynamic dispatch.
56
+ internalInstance.restoreControlledState();
57
+ },
58
+};
59
+
60
+module.exports = ReactControlledComponent;
src/renderers/shared/stack/reconciler/ReactUpdates.js
+2
-27
@@ -21,8 +21,6 @@ var invariant = require('invariant');
21
22
var dirtyComponents = [];
23
var updateBatchNumber = 0;
24
-var asapCallbackQueue = CallbackQueue.getPooled();
25
-var asapEnqueued = false;
24
25
var batchingStrategy = null;
26
@@ -180,21 +178,13 @@ var flushBatchedUpdates = function() {
178
// ReactUpdatesFlushTransaction's wrappers will clear the dirtyComponents
179
// array and perform any updates enqueued by mount-ready handlers (i.e.,
180
// componentDidUpdate) but we need to check here too in order to catch
183
- // updates enqueued by setState callbacks and asap calls.
184
- while (dirtyComponents.length || asapEnqueued) {
181
+ // updates enqueued by setState callbacks.
182
+ while (dirtyComponents.length) {
183
if (dirtyComponents.length) {
184
var transaction = ReactUpdatesFlushTransaction.getPooled();
185
transaction.perform(runBatchedUpdates, null, transaction);
186
ReactUpdatesFlushTransaction.release(transaction);
187
}
190
-
191
- if (asapEnqueued) {
192
- asapEnqueued = false;
193
- var queue = asapCallbackQueue;
194
- asapCallbackQueue = CallbackQueue.getPooled();
195
- queue.notifyAll();
196
- CallbackQueue.release(queue);
197
- }
188
}
189
};
190
@@ -222,20 +212,6 @@ function enqueueUpdate(component) {
212
}
213
}
214
225
-/**
226
- * Enqueue a callback to be run at the end of the current batching cycle. Throws
227
- * if no updates are currently being performed.
228
- */
229
-function asap(callback, context) {
230
- invariant(
231
- batchingStrategy.isBatchingUpdates,
232
- 'ReactUpdates.asap: Can\'t enqueue an asap callback in a context where' +
233
- 'updates are not being batched.'
234
- );
235
- asapCallbackQueue.enqueue(callback, context);
236
- asapEnqueued = true;
237
-}
238
-
215
var ReactUpdatesInjection = {
216
injectReconcileTransaction: function(ReconcileTransaction) {
217
invariant(
@@ -279,7 +255,6 @@ var ReactUpdates = {
255
enqueueUpdate: enqueueUpdate,
256
flushBatchedUpdates: flushBatchedUpdates,
257
injection: ReactUpdatesInjection,
282
- asap: asap,
258
};
259
260
module.exports = ReactUpdates;
src/renderers/shared/stack/reconciler/__tests__/ReactUpdates-test.js
-77
@@ -810,83 +810,6 @@ describe('ReactUpdates', () => {
810
expect(callbackCount).toBe(1);
811
});
812
813
- it('calls asap callbacks properly', () => {
814
- var callbackCount = 0;
815
-
816
- class A extends React.Component {
817
- render() {
818
- return <div />;
819
- }
820
-
821
- componentDidUpdate() {
822
- ReactUpdates.asap(function() {
823
- expect(this).toBe(component);
824
- callbackCount++;
825
- ReactUpdates.asap(function() {
826
- callbackCount++;
827
- });
828
- expect(callbackCount).toBe(1);
829
- }, component);
830
- expect(callbackCount).toBe(0);
831
- }
832
- }
833
-
834
- var component = ReactTestUtils.renderIntoDocument(<A />);
835
- component.forceUpdate();
836
- expect(callbackCount).toBe(2);
837
- });
838
-
839
- it('calls asap callbacks with queued updates', () => {
840
- var log = [];
841
-
842
- class A extends React.Component {
843
- state = {updates: 0};
844
-
845
- render() {
846
- log.push('render-' + this.state.updates);
847
- return <div />;
848
- }
849
-
850
- componentDidUpdate() {
851
- if (this.state.updates === 1) {
852
- ReactUpdates.asap(function() {
853
- this.setState({updates: 2}, function() {
854
- ReactUpdates.asap(function() {
855
- log.push('asap-1.2');
856
- });
857
- log.push('setState-cb');
858
- });
859
- log.push('asap-1.1');
860
- }, this);
861
- } else if (this.state.updates === 2) {
862
- ReactUpdates.asap(function() {
863
- log.push('asap-2');
864
- });
865
- }
866
- log.push('didUpdate-' + this.state.updates);
867
- }
868
- }
869
-
870
- var component = ReactTestUtils.renderIntoDocument(<A />);
871
- component.setState({updates: 1});
872
- expect(log).toEqual([
873
- 'render-0',
874
- // We do the first update...
875
- 'render-1',
876
- 'didUpdate-1',
877
- // ...which calls asap and enqueues a second update...
878
- 'asap-1.1',
879
- // ...which runs and enqueues the asap-2 log in its didUpdate...
880
- 'render-2',
881
- 'didUpdate-2',
882
- // ...and runs the setState callback, which enqueues the log for
883
- // asap-1.2.
884
- 'setState-cb',
885
- 'asap-2',
886
- 'asap-1.2',
887
- ]);
888
- });
889
-
813
it('does not call render after a component as been deleted', () => {
814
var renderCount = 0;
815
var componentB = null;
src/test/ReactTestUtils.js
+10
-1
@@ -16,6 +16,7 @@ var EventPluginHub = require('EventPluginHub');
16
var EventPluginRegistry = require('EventPluginRegistry');
17
var EventPropagators = require('EventPropagators');
18
var React = require('React');
19
+var ReactControlledComponent = require('ReactControlledComponent');
20
var ReactDOM = require('ReactDOM');
21
var ReactDOMComponentTree = require('ReactDOMComponentTree');
22
var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
@@ -431,12 +432,14 @@ function makeSimulator(eventType) {
432
433
// We don't use SyntheticEvent.getPooled in order to not have to worry about
434
// properly destroying any properties assigned from `eventData` upon release
435
+ var targetInst = ReactDOMComponentTree.getInstanceFromNode(node);
436
var event = new SyntheticEvent(
437
dispatchConfig,
436
- ReactDOMComponentTree.getInstanceFromNode(node),
438
+ targetInst,
439
fakeNativeEvent,
440
node
441
);
442
+
443
// Since we aren't using pooling, always persist the event. This will make
444
// sure it's marked and won't warn when setting additional properties.
445
event.persist();
@@ -452,6 +455,12 @@ function makeSimulator(eventType) {
455
EventPluginHub.enqueueEvents(event);
456
EventPluginHub.processEventQueue(true);
457
});
458
+ // Normally extractEvent enqueues a state restore, but we'll just always do
459
+ // that.
460
+ ReactControlledComponent.enqueueStateRestore();
461
+ if (targetInst) {
462
+ ReactControlledComponent.restoreStateIfNeeded(targetInst);
463
+ }
464
};
465
}
466