From 1d49237299fdbf8006c0eab8ebb6ba1386434036 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 4 Aug 2016 17:56:48 -0700 Subject: [PATCH] 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. --- src/renderers/shared/fiber/ReactFiber.js | 5 ++++ .../shared/fiber/ReactFiberBeginWork.js | 12 ++++++++ .../shared/fiber/ReactFiberCommitWork.js | 9 ++++++ .../shared/fiber/ReactFiberCompleteWork.js | 11 +++++-- .../shared/fiber/ReactFiberUpdateQueue.js | 20 ++++++++++--- .../ReactIncrementalSideEffects-test.js | 30 +++++++++++++++++++ 6 files changed, 80 insertions(+), 7 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index 495c615c95..7dadfbe47a 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/src/renderers/shared/fiber/ReactFiber.js @@ -81,6 +81,8 @@ export type Fiber = Instance & { updateQueue: ?UpdateQueue, // The state used to create the output. This is a full state object. memoizedState: any, + // Linked list of callbacks to call after updates are committed. + callbackList: ?UpdateQueue, // Output is the return value of this fiber, or a linked list of return values // if this returns multiple values. Such as a fragment. 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 { memoizedProps: null, updateQueue: null, memoizedState: null, + callbackList: null, output: null, nextEffect: null, @@ -200,6 +203,7 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi alt.ref = fiber.ref; alt.pendingProps = fiber.pendingProps; // TODO: Pass as argument. alt.updateQueue = fiber.updateQueue; + alt.callbackList = fiber.callbackList; alt.pendingWorkPriority = priorityLevel; alt.child = fiber.child; @@ -226,6 +230,7 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi // TODO: Pass in the new pendingProps as an argument maybe? alt.pendingProps = fiber.pendingProps; alt.updateQueue = fiber.updateQueue; + alt.callbackList = fiber.callbackList; alt.pendingWorkPriority = priorityLevel; alt.memoizedProps = fiber.memoizedProps; diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index 36ee2b9d78..c49f16ce91 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -44,6 +44,7 @@ var { var { createUpdateQueue, addToQueue, + addCallbackToQueue, mergeUpdateQueue, } = require('ReactFiberUpdateQueue'); var ReactInstanceMap = require('ReactInstanceMap'); @@ -156,6 +157,17 @@ module.exports = function(config : HostConfig, getSchedu createUpdateQueue(partialState); scheduleUpdate(fiber, updateQueue, LowPriority); }, + enqueueCallback(instance, callback) { + const fiber = ReactInstanceMap.get(instance); + let updateQueue = fiber.updateQueue ? + fiber.updateQueue : + createUpdateQueue(null); + addCallbackToQueue(updateQueue, callback); + fiber.updateQueue = updateQueue; + if (fiber.alternate) { + fiber.alternate.updateQueue = updateQueue; + } + }, }; function updateClassComponent(current : ?Fiber, workInProgress : Fiber) { diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 2e3e8ae3d0..d859003383 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -22,6 +22,7 @@ var { HostContainer, HostComponent, } = ReactTypeOfWork; +var { callCallbacks } = require('ReactFiberUpdateQueue'); module.exports = function(config : HostConfig) { @@ -31,6 +32,14 @@ module.exports = function(config : HostConfig) { function commitWork(current : ?Fiber, finishedWork : Fiber) : void { switch (finishedWork.tag) { case ClassComponent: { + if (finishedWork.callbackList) { + const { callbackList } = finishedWork; + finishedWork.callbackList = null; + if (finishedWork.alternate) { + finishedWork.alternate.callbackList = null; + } + callCallbacks(callbackList, finishedWork.stateNode); + } // TODO: Fire componentDidMount/componentDidUpdate, update refs return; } diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index fdeb1f5e56..6d6f9598c4 100644 --- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js +++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js @@ -46,7 +46,6 @@ module.exports = function(config : HostConfig) { } } - /* // TODO: It's possible this will create layout thrash issues because mutations // of the DOM and life-cycles are interleaved. E.g. if a componentDidMount // of a sibling reads, then the next sibling updates and reads etc. @@ -59,7 +58,6 @@ module.exports = function(config : HostConfig) { } workInProgress.lastEffect = workInProgress; } - */ function transferOutput(child : ?Fiber, returnFiber : Fiber) { // If we have a single result, we just pass that through as the output to @@ -133,9 +131,16 @@ module.exports = function(config : HostConfig) { case ClassComponent: transferOutput(workInProgress.child, workInProgress); // Don't use the state queue to compute the memoized state. We already - // merged it and assigned it to the instance. Copy it from there. + // merged it and assigned it to the instance. Transfer it from there. const state = workInProgress.stateNode.state; workInProgress.memoizedState = state; + // Transfer update queue to callbackList field so callbacks can be + // called during commit phase. + workInProgress.callbackList = workInProgress.updateQueue; + if (current) { + current.callbackList = workInProgress.callbackList; + } + markForPostEffect(workInProgress); return null; case HostContainer: transferOutput(workInProgress.child, workInProgress); diff --git a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js index 57329b2522..159b0ceb09 100644 --- a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js +++ b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js @@ -15,6 +15,7 @@ type UpdateQueueNode = { partialState: any, callback: ?Function, + callbackWasCalled: boolean, next: ?UpdateQueueNode, }; @@ -26,6 +27,7 @@ exports.createUpdateQueue = function(partialState : mixed) : UpdateQueue { const queue = { partialState, callback: null, + callbackWasCalled: false, next: null, tail: (null : any), }; @@ -37,6 +39,7 @@ exports.addToQueue = function(queue : UpdateQueue, partialState : mixed) : Updat const node = { partialState, callback: null, + callbackWasCalled: false, next: null, }; queue.tail.next = node; @@ -44,12 +47,21 @@ exports.addToQueue = function(queue : UpdateQueue, partialState : mixed) : Updat return queue; }; -exports.callCallbacks = function(queue : UpdateQueue, partialState : mixed) { +exports.addCallbackToQueue = function(queue : UpdateQueue, callback: Function) : UpdateQueue { + if (queue.tail.callback) { + // If the tail already as a callback, add an empty node to queue + exports.addToQueue(queue, null); + } + queue.tail.callback = callback; + return queue; +}; + +exports.callCallbacks = function(queue : UpdateQueue, context : any) { let node : ?UpdateQueueNode = queue; while (node) { - if (node.callback) { - const { callback } = node; - callback(); + if (node.callback && !node.callbackWasCalled) { + node.callbackWasCalled = true; + node.callback.call(context); } node = node.next; } diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js index 63983720df..f49f09592e 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js @@ -354,4 +354,34 @@ describe('ReactIncrementalSideEffects', () => { // moves to "current" without flushing due to having lower priority. Does this // even happen? Maybe a child doesn't get processed because it is lower prio? + it('calls callback after update is flushed', () => { + let instance; + class Foo extends React.Component { + constructor() { + super(); + instance = this; + this.state = { text: 'foo' }; + } + render() { + return ; + } + } + + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.root.children).toEqual([ + span('foo'), + ]); + let called = false; + instance.setState({ text: 'bar' }, () => { + expect(ReactNoop.root.children).toEqual([ + span('bar'), + ]); + called = true; + }); + ReactNoop.flush(); + expect(called).toBe(true); + }); + + // TODO: Test that callbacks are not lost if an update is preempted. });