@samitouri / QOS-React / commits / 4804518c26

Handle nested controlled events (#8251)

I came up with a contrived case of where nested controlled events could fire within the same batch - but on different targets. I think we came to the conclusion that controlled values typically cannot use preventDefault so it is ok that they don't flush until after the event has finished. So therefore we accumulate a queue of all the nested targets within a batch and then restore state on all of them. I'm still skeptical that this is the correct way to do controlled values. The reason we have to do them in a single event loop is because when you type, the sequence of values that get accepted or not can matter. I wonder if there is a scenario we can come up with where you can fire multiple inner events in an event loop and end up with batching causing problems. This effectively just reimplements asap again but with no allocations for a single target and no closure allocations.

Sebastian Markbåge committed Nov 11, 2016 at 16:49 UTC 4804518c26ce86e725a9330cc3e8eb7b873ed417
7 files changed +116 -51
scripts/fiber/tests-failing.txt
+1
@@ -254,6 +254,7 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMIframe-test.js
254 src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
255 * should properly control a value even if no event listener exists
256 * should control a value in reentrant events
257 +* should control values in reentrant events with different targets
258 * should display `defaultValue` of number 0
259 * should display "true" for `defaultValue` of `true`
260 * should display "false" for `defaultValue` of `false`
src/renderers/dom/shared/ReactEventListener.js
+1 -5
@@ -171,11 +171,7 @@ var ReactEventListener = {
171 try {
172 // Event queue being processed in the same cycle allows
173 // `preventDefault`.
174 - ReactGenericBatching.batchedUpdatesWithControlledTarget(
175 - handleTopLevelImpl,
176 - bookKeeping,
177 - bookKeeping.targetInst
178 - );
174 + ReactGenericBatching.batchedUpdates(handleTopLevelImpl, bookKeeping);
175 } finally {
176 TopLevelCallbackBookKeeping.release(bookKeeping);
177 }
src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js
+2 -6
@@ -52,7 +52,7 @@ function createAndAccumulateChangeEvent(inst, nativeEvent, target) {
52 );
53 event.type = 'change';
54 // Flag this event loop as needing state restore.
55 - ReactControlledComponent.enqueueStateRestore();
55 + ReactControlledComponent.enqueueStateRestore(inst);
56 EventPropagators.accumulateTwoPhaseDispatches(event);
57 return event;
58 }
@@ -101,11 +101,7 @@ function manualDispatchChangeEvent(nativeEvent) {
101 // components don't work properly in conjunction with event bubbling because
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.batchedUpdatesWithControlledTarget(
105 - runEventInBatch,
106 - event,
107 - activeElementInst
108 - );
104 + ReactGenericBatching.batchedUpdates(runEventInBatch, event);
105 }
106
107 function runEventInBatch(event) {
src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
+54
@@ -132,6 +132,60 @@ describe('ReactDOMInput', () => {
132 document.body.removeChild(container);
133 });
134
135 + it('should control values in reentrant events with different targets', () => {
136 + // This must use the native event dispatching. If we simulate, we will
137 + // bypass the lazy event attachment system so we won't actually test this.
138 + var inputEvent = document.createEvent('Event');
139 + inputEvent.initEvent('input', true, true);
140 +
141 + class ControlledInputs extends React.Component {
142 + state = { value: 'lion' };
143 + a = null;
144 + b = null;
145 + change(newValue) {
146 + // This click will change the checkbox's value to false. Then it will
147 + // invoke an inner change event. When we finally, flush, we need to
148 + // reset the checkbox's value to true since that is its controlled
149 + // value.
150 + this.b.click();
151 + }
152 + render() {
153 + return (
154 + <div>
155 + <input
156 + type="text"
157 + ref={n => this.a = n}
158 + value="lion"
159 + onChange={e => this.change(e.target.value)}
160 + />
161 + <input
162 + type="checkbox"
163 + ref={n => this.b = n}
164 + checked={true}
165 + />
166 + </div>
167 + );
168 + }
169 + }
170 +
171 + var container = document.createElement('div');
172 + var instance = ReactDOM.render(<ControlledInputs />, container);
173 +
174 + // We need it to be in the body to test native event dispatching.
175 + document.body.appendChild(container);
176 +
177 + // Simulate a native keyup event
178 + setUntrackedValue(instance.a, 'giraffe');
179 + instance.a.dispatchEvent(inputEvent);
180 +
181 + // These should now both have been restored to their controlled value.
182 +
183 + expect(instance.a.value).toBe('lion');
184 + expect(instance.b.checked).toBe(true);
185 +
186 + document.body.removeChild(container);
187 + });
188 +
189 it('should display `defaultValue` of number 0', () => {
190 var stub = <input type="text" defaultValue={0} />;
191 stub = ReactTestUtils.renderIntoDocument(stub);
src/renderers/shared/shared/event/ReactControlledComponent.js
+42 -20
@@ -25,35 +25,57 @@ var ReactControlledComponentInjection = {
25 },
26 };
27
28 -var needsRestoreState = false;
28 +var restoreTarget = null;
29 +var restoreQueue = null;
30 +
31 +function restoreStateOfTarget(internalInstance) {
32 + if (typeof internalInstance.tag === 'number') {
33 + invariant(
34 + fiberHostComponent &&
35 + typeof fiberHostComponent.restoreControlledState === 'function',
36 + 'Fiber needs to be injected to handle a fiber target for controlled ' +
37 + 'events.'
38 + );
39 + fiberHostComponent.restoreControlledState(internalInstance);
40 + }
41 + invariant(
42 + typeof internalInstance.restoreControlledState === 'function',
43 + 'The internal instance must be a React host component.'
44 + );
45 + // If it is not a Fiber, we can just use dynamic dispatch.
46 + internalInstance.restoreControlledState();
47 +}
48
49 var ReactControlledComponent = {
50 injection: ReactControlledComponentInjection,
51
33 - enqueueStateRestore() {
34 - needsRestoreState = true;
52 + enqueueStateRestore(target) {
53 + if (restoreTarget) {
54 + if (restoreQueue) {
55 + restoreQueue.push(target);
56 + } else {
57 + restoreQueue = [target];
58 + }
59 + } else {
60 + restoreTarget = target;
61 + }
62 },
63
37 - restoreStateIfNeeded(internalInstance) {
38 - if (!needsRestoreState) {
64 + restoreStateIfNeeded() {
65 + if (!restoreTarget) {
66 return;
67 }
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);
68 + var target = restoreTarget;
69 + var queuedTargets = restoreQueue;
70 + restoreTarget = null;
71 + restoreQueue = null;
72 +
73 + restoreStateOfTarget(target);
74 + if (queuedTargets) {
75 + for (var i = 0; i < queuedTargets.length; i++) {
76 + restoreStateOfTarget(queuedTargets[i]);
77 + }
78 }
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();
79 },
80 };
81
src/renderers/shared/shared/event/ReactGenericBatching.js
+10 -12
@@ -38,26 +38,25 @@ function batchedUpdates(fn, bookkeeping) {
38 stackBatchedUpdates(performFiberBatchedUpdates, fn, bookkeeping);
39 }
40
41 -var isBatching = false;
42 -function batchedUpdatesWithControlledTarget(fn, bookkeeping, target) {
43 - if (isBatching) {
44 - // TODO: If this target is not the same as the one currently batched,
45 - // we'll drop it.
41 +var isNestingBatched = false;
42 +function batchedUpdatesWithControlledComponents(fn, bookkeeping) {
43 + if (isNestingBatched) {
44 + // If we are currently inside another batch, we need to wait until it
45 + // fully completes before restoring state. Therefore, we add the target to
46 + // a queue of work.
47 batchedUpdates(fn, bookkeeping);
48 return;
49 }
49 - isBatching = true;
50 + isNestingBatched = true;
51 try {
52 batchedUpdates(fn, bookkeeping);
53 } finally {
53 - isBatching = false;
54 - }
55 - if (target) {
54 // Here we wait until all updates have propagated, which is important
55 // when using controlled components within layers:
56 // https://github.com/facebook/react/issues/1698
57 // Then we restore state of any controlled component.
60 - ReactControlledComponent.restoreStateIfNeeded(target);
58 + isNestingBatched = false;
59 + ReactControlledComponent.restoreStateIfNeeded();
60 }
61 }
62
@@ -71,8 +70,7 @@ var ReactGenericBatchingInjection = {
70 };
71
72 var ReactGenericBatching = {
74 - batchedUpdates,
75 - batchedUpdatesWithControlledTarget,
73 + batchedUpdates: batchedUpdatesWithControlledComponents,
74 injection: ReactGenericBatchingInjection,
75 };
76
src/test/ReactTestUtils.js
+6 -8
@@ -22,7 +22,7 @@ var ReactDOMComponentTree = require('ReactDOMComponentTree');
22 var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
23 var ReactInstanceMap = require('ReactInstanceMap');
24 var ReactTypeOfWork = require('ReactTypeOfWork');
25 -var ReactUpdates = require('ReactUpdates');
25 +var ReactGenericBatching = require('ReactGenericBatching');
26 var SyntheticEvent = require('SyntheticEvent');
27 var ReactShallowRenderer = require('ReactShallowRenderer');
28
@@ -465,16 +465,14 @@ function makeSimulator(eventType) {
465 EventPropagators.accumulateDirectDispatches(event);
466 }
467
468 - ReactUpdates.batchedUpdates(function() {
468 + ReactGenericBatching.batchedUpdates(function() {
469 + // Normally extractEvent enqueues a state restore, but we'll just always
470 + // do that since we we're by-passing it here.
471 + ReactControlledComponent.enqueueStateRestore(targetInst);
472 +
473 EventPluginHub.enqueueEvents(event);
474 EventPluginHub.processEventQueue(true);
475 });
472 - // Normally extractEvent enqueues a state restore, but we'll just always do
473 - // that.
474 - ReactControlledComponent.enqueueStateRestore();
475 - if (targetInst) {
476 - ReactControlledComponent.restoreStateIfNeeded(targetInst);
477 - }
476 };
477 }
478