From e59e5b280f4b433d6c8cd7821fa1f64ad7847e35 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 7 Oct 2016 15:46:43 -0700 Subject: [PATCH] Invoke all null ref calls before any new ref calls This reorganizes the two commit passes so that all host environment mutations happens before any life-cycles. That means that the tree is guaranteed to be in a consistent state at that point so that you can read layout etc. This also lets us to detach all refs in the same pass so that when they get invoked with new instances, that happens after it has been reset. --- .../shared/fiber/ReactFiberCommitWork.js | 77 ++++++++++++------- .../shared/fiber/ReactFiberScheduler.js | 35 ++++++--- .../ReactIncrementalSideEffects-test.js | 9 +-- 3 files changed, 76 insertions(+), 45 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 2591361128..837f12d00c 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -47,18 +47,18 @@ module.exports = function(config : HostConfig) { } } - function attachRef(current : ?Fiber, finishedWork : Fiber, instance : any) { - const ref = finishedWork.ref; + function detachRefIfNeeded(current : ?Fiber, finishedWork : Fiber) { if (current) { const currentRef = current.ref; - if (currentRef && currentRef !== ref) { - // TODO: This needs to be done in a separate pass before any other refs - // gets resolved. Otherwise we might invoke them in the wrong order - // when the same ref switches between two components. + if (currentRef && currentRef !== finishedWork.ref) { currentRef(null); } } - if (ref) { + } + + function attachRef(current : ?Fiber, finishedWork : Fiber, instance : any) { + const ref = finishedWork.ref; + if (ref && (!current || current.ref !== ref)) { ref(instance); } } @@ -239,6 +239,46 @@ module.exports = function(config : HostConfig) { } function commitWork(current : ?Fiber, finishedWork : Fiber) : void { + switch (finishedWork.tag) { + case ClassComponent: { + detachRefIfNeeded(current, finishedWork); + return; + } + case HostContainer: { + // TODO: Attach children to root container. + const children = finishedWork.output; + const root : FiberRoot = finishedWork.stateNode; + const containerInfo : C = root.containerInfo; + updateContainer(containerInfo, children); + return; + } + case HostComponent: { + const instance : I = finishedWork.stateNode; + if (instance != null && current) { + // Commit the work prepared earlier. + const newProps = finishedWork.memoizedProps; + const oldProps = current.memoizedProps; + commitUpdate(instance, oldProps, newProps); + } + detachRefIfNeeded(current, finishedWork); + return; + } + case HostText: { + if (finishedWork.stateNode == null || !current) { + throw new Error('This should only be done during updates.'); + } + const textInstance : TI = finishedWork.stateNode; + const newText : string = finishedWork.memoizedProps; + const oldText : string = current.memoizedProps; + commitTextUpdate(textInstance, oldText, newText); + return; + } + default: + throw new Error('This unit of work tag should not have side-effects.'); + } + } + + function commitLifeCycles(current : ?Fiber, finishedWork : Fiber) : void { switch (finishedWork.tag) { case ClassComponent: { const instance = finishedWork.stateNode; @@ -270,33 +310,13 @@ module.exports = function(config : HostConfig) { attachRef(current, finishedWork, instance); return; } - case HostContainer: { - // TODO: Attach children to root container. - const children = finishedWork.output; - const root : FiberRoot = finishedWork.stateNode; - const containerInfo : C = root.containerInfo; - updateContainer(containerInfo, children); - return; - } case HostComponent: { const instance : I = finishedWork.stateNode; - if (instance != null && current) { - // Commit the work prepared earlier. - const newProps = finishedWork.memoizedProps; - const oldProps = current.memoizedProps; - commitUpdate(instance, oldProps, newProps); - } attachRef(current, finishedWork, instance); return; } case HostText: { - if (finishedWork.stateNode == null || !current) { - throw new Error('This should only be done during updates.'); - } - const textInstance : TI = finishedWork.stateNode; - const newText : string = finishedWork.memoizedProps; - const oldText : string = current.memoizedProps; - commitTextUpdate(textInstance, oldText, newText); + // We have no life-cycles associated with text. return; } default: @@ -308,6 +328,7 @@ module.exports = function(config : HostConfig) { commitInsertion, commitDeletion, commitWork, + commitLifeCycles, }; }; diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 1348e6037a..a1e0382ef7 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -54,7 +54,8 @@ module.exports = function(config : HostConfig) { const { beginWork } = ReactFiberBeginWork(config, getScheduler); const { completeWork } = ReactFiberCompleteWork(config); - const { commitInsertion, commitDeletion, commitWork } = ReactFiberCommitWork(config); + const { commitInsertion, commitDeletion, commitWork, commitLifeCycles } = + ReactFiberCommitWork(config); const scheduleAnimationCallback = config.scheduleAnimationCallback; const scheduleDeferredCallback = config.scheduleDeferredCallback; @@ -112,31 +113,43 @@ module.exports = function(config : HostConfig) { // Commit all the side-effects within a tree. // TODO: Error handling. - // First, we'll perform all the host insertion and deletion effects. + // First, we'll perform all the host insertions, updates, deletions and + // ref unmounts. let effectfulFiber = finishedWork.firstEffect; while (effectfulFiber) { switch (effectfulFiber.effectTag) { - // TODO: Should we commit host updates here? Otherwise update to parent - // host components are not visible in life-cycles. Such as when you read - // layout information. - case Placement: - case PlacementAndUpdate: + case Placement: { commitInsertion(effectfulFiber); break; - case Deletion: + } + case PlacementAndUpdate: { + commitInsertion(effectfulFiber); + const current = effectfulFiber.alternate; + commitWork(current, effectfulFiber); + break; + } + case Update: { + const current = effectfulFiber.alternate; + commitWork(current, effectfulFiber); + break; + } + case Deletion: { commitDeletion(effectfulFiber); break; + } } effectfulFiber = effectfulFiber.nextEffect; } - // Next, we'll perform all other effects. + // Next, we'll perform all life-cycles and ref callbacks. Life-cycles + // happens as a separate pass so that all effects in the entire tree have + // already been invoked. effectfulFiber = finishedWork.firstEffect; while (effectfulFiber) { - const current = effectfulFiber.alternate; if (effectfulFiber.effectTag === Update || effectfulFiber.effectTag === PlacementAndUpdate) { - commitWork(current, effectfulFiber); + const current = effectfulFiber.alternate; + commitLifeCycles(current, effectfulFiber); } const next = effectfulFiber.nextEffect; // Ensure that we clean these up so that we don't accidentally keep them. diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js index 382f947fc5..3347b7212f 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js @@ -680,14 +680,11 @@ describe('ReactIncrementalSideEffects', () => { ReactNoop.render(); ReactNoop.flush(); expect(ops).toEqual([ - // TODO: All detach should happen first. Currently they're interleaved. - // detach + // detach all refs that switched handlers first. null, - // reattach + null, + // reattach as a separate phase classInstance, - // detach - null, - // reattach div(), ]);