@samitouri / QOS-React / commits / 1d49237299

Update callbacks

Callbacks are stored on the same queue as updates. They care called during the commit phase, after the updates have been flushed. Because the update queue is cleared during the completion phase (before commit), a new field has been added to fiber called callbackList. The queue is transferred from updateQueue to callbackList during completion. During commit, the list is reset. Need a test to confirm that callbacks are not lost if an update is preempted.

Andrew Clark committed Aug 4, 2016 at 17:56 UTC 1d49237299fdbf8006c0eab8ebb6ba1386434036
6 files changed +80 -7
src/renderers/shared/fiber/ReactFiber.js
+5
@@ -81,6 +81,8 @@ export type Fiber = Instance & {
81 updateQueue: ?UpdateQueue,
82 // The state used to create the output. This is a full state object.
83 memoizedState: any,
84 + // Linked list of callbacks to call after updates are committed.
85 + callbackList: ?UpdateQueue,
86 // Output is the return value of this fiber, or a linked list of return values
87 // if this returns multiple values. Such as a fragment.
88 output: any, // This type will be more specific once we overload the tag.
@@ -158,6 +160,7 @@ var createFiber = function(tag : TypeOfWork, key : null | string) : Fiber {
160 memoizedProps: null,
161 updateQueue: null,
162 memoizedState: null,
163 + callbackList: null,
164 output: null,
165
166 nextEffect: null,
@@ -200,6 +203,7 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi
203 alt.ref = fiber.ref;
204 alt.pendingProps = fiber.pendingProps; // TODO: Pass as argument.
205 alt.updateQueue = fiber.updateQueue;
206 + alt.callbackList = fiber.callbackList;
207 alt.pendingWorkPriority = priorityLevel;
208
209 alt.child = fiber.child;
@@ -226,6 +230,7 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi
230 // TODO: Pass in the new pendingProps as an argument maybe?
231 alt.pendingProps = fiber.pendingProps;
232 alt.updateQueue = fiber.updateQueue;
233 + alt.callbackList = fiber.callbackList;
234 alt.pendingWorkPriority = priorityLevel;
235
236 alt.memoizedProps = fiber.memoizedProps;
src/renderers/shared/fiber/ReactFiberBeginWork.js
+12
@@ -44,6 +44,7 @@ var {
44 var {
45 createUpdateQueue,
46 addToQueue,
47 + addCallbackToQueue,
48 mergeUpdateQueue,
49 } = require('ReactFiberUpdateQueue');
50 var ReactInstanceMap = require('ReactInstanceMap');
@@ -156,6 +157,17 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>, getSchedu
157 createUpdateQueue(partialState);
158 scheduleUpdate(fiber, updateQueue, LowPriority);
159 },
160 + enqueueCallback(instance, callback) {
161 + const fiber = ReactInstanceMap.get(instance);
162 + let updateQueue = fiber.updateQueue ?
163 + fiber.updateQueue :
164 + createUpdateQueue(null);
165 + addCallbackToQueue(updateQueue, callback);
166 + fiber.updateQueue = updateQueue;
167 + if (fiber.alternate) {
168 + fiber.alternate.updateQueue = updateQueue;
169 + }
170 + },
171 };
172
173 function updateClassComponent(current : ?Fiber, workInProgress : Fiber) {
src/renderers/shared/fiber/ReactFiberCommitWork.js
+9
@@ -22,6 +22,7 @@ var {
22 HostContainer,
23 HostComponent,
24 } = ReactTypeOfWork;
25 +var { callCallbacks } = require('ReactFiberUpdateQueue');
26
27 module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
28
@@ -31,6 +32,14 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
32 function commitWork(current : ?Fiber, finishedWork : Fiber) : void {
33 switch (finishedWork.tag) {
34 case ClassComponent: {
35 + if (finishedWork.callbackList) {
36 + const { callbackList } = finishedWork;
37 + finishedWork.callbackList = null;
38 + if (finishedWork.alternate) {
39 + finishedWork.alternate.callbackList = null;
40 + }
41 + callCallbacks(callbackList, finishedWork.stateNode);
42 + }
43 // TODO: Fire componentDidMount/componentDidUpdate, update refs
44 return;
45 }
src/renderers/shared/fiber/ReactFiberCompleteWork.js
+8 -3
@@ -46,7 +46,6 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
46 }
47 }
48
49 - /*
49 // TODO: It's possible this will create layout thrash issues because mutations
50 // of the DOM and life-cycles are interleaved. E.g. if a componentDidMount
51 // of a sibling reads, then the next sibling updates and reads etc.
@@ -59,7 +58,6 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
58 }
59 workInProgress.lastEffect = workInProgress;
60 }
62 - */
61
62 function transferOutput(child : ?Fiber, returnFiber : Fiber) {
63 // If we have a single result, we just pass that through as the output to
@@ -133,9 +131,16 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
131 case ClassComponent:
132 transferOutput(workInProgress.child, workInProgress);
133 // Don't use the state queue to compute the memoized state. We already
136 - // merged it and assigned it to the instance. Copy it from there.
134 + // merged it and assigned it to the instance. Transfer it from there.
135 const state = workInProgress.stateNode.state;
136 workInProgress.memoizedState = state;
137 + // Transfer update queue to callbackList field so callbacks can be
138 + // called during commit phase.
139 + workInProgress.callbackList = workInProgress.updateQueue;
140 + if (current) {
141 + current.callbackList = workInProgress.callbackList;
142 + }
143 + markForPostEffect(workInProgress);
144 return null;
145 case HostContainer:
146 transferOutput(workInProgress.child, workInProgress);
src/renderers/shared/fiber/ReactFiberUpdateQueue.js
+16 -4
@@ -15,6 +15,7 @@
15 type UpdateQueueNode = {
16 partialState: any,
17 callback: ?Function,
18 + callbackWasCalled: boolean,
19 next: ?UpdateQueueNode,
20 };
21
@@ -26,6 +27,7 @@ exports.createUpdateQueue = function(partialState : mixed) : UpdateQueue {
27 const queue = {
28 partialState,
29 callback: null,
30 + callbackWasCalled: false,
31 next: null,
32 tail: (null : any),
33 };
@@ -37,6 +39,7 @@ exports.addToQueue = function(queue : UpdateQueue, partialState : mixed) : Updat
39 const node = {
40 partialState,
41 callback: null,
42 + callbackWasCalled: false,
43 next: null,
44 };
45 queue.tail.next = node;
@@ -44,12 +47,21 @@ exports.addToQueue = function(queue : UpdateQueue, partialState : mixed) : Updat
47 return queue;
48 };
49
47 -exports.callCallbacks = function(queue : UpdateQueue, partialState : mixed) {
50 +exports.addCallbackToQueue = function(queue : UpdateQueue, callback: Function) : UpdateQueue {
51 + if (queue.tail.callback) {
52 + // If the tail already as a callback, add an empty node to queue
53 + exports.addToQueue(queue, null);
54 + }
55 + queue.tail.callback = callback;
56 + return queue;
57 +};
58 +
59 +exports.callCallbacks = function(queue : UpdateQueue, context : any) {
60 let node : ?UpdateQueueNode = queue;
61 while (node) {
50 - if (node.callback) {
51 - const { callback } = node;
52 - callback();
62 + if (node.callback && !node.callbackWasCalled) {
63 + node.callbackWasCalled = true;
64 + node.callback.call(context);
65 }
66 node = node.next;
67 }
src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js
+30
@@ -354,4 +354,34 @@ describe('ReactIncrementalSideEffects', () => {
354 // moves to "current" without flushing due to having lower priority. Does this
355 // even happen? Maybe a child doesn't get processed because it is lower prio?
356
357 + it('calls callback after update is flushed', () => {
358 + let instance;
359 + class Foo extends React.Component {
360 + constructor() {
361 + super();
362 + instance = this;
363 + this.state = { text: 'foo' };
364 + }
365 + render() {
366 + return <span prop={this.state.text} />;
367 + }
368 + }
369 +
370 + ReactNoop.render(<Foo />);
371 + ReactNoop.flush();
372 + expect(ReactNoop.root.children).toEqual([
373 + span('foo'),
374 + ]);
375 + let called = false;
376 + instance.setState({ text: 'bar' }, () => {
377 + expect(ReactNoop.root.children).toEqual([
378 + span('bar'),
379 + ]);
380 + called = true;
381 + });
382 + ReactNoop.flush();
383 + expect(called).toBe(true);
384 + });
385 +
386 + // TODO: Test that callbacks are not lost if an update is preempted.
387 });