From dcc02dd0f12fb4b9b4b910aef6c5808dc761dc22 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 15 Jun 2017 19:28:59 -0700 Subject: [PATCH] Task work inside batched updates is always sync, even for initial mounts Behavior now matches Stack. It's unfortunate that this prevents us from unifying SynchronousPriority and TaskPriority. --- scripts/fiber/tests-passing.txt | 2 + .../dom/shared/__tests__/ReactMount-test.js | 35 ++++++++ .../shared/fiber/ReactFiberScheduler.js | 81 ++++++++++++++++--- .../__tests__/ReactIncrementalPerf-test.js | 16 ++++ .../ReactIncrementalPerf-test.js.snap | 14 ++++ 5 files changed, 135 insertions(+), 13 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 143f9725a1..aa0c55ba14 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1309,6 +1309,7 @@ src/renderers/dom/shared/__tests__/ReactMount-test.js * should warn if render removes React-rendered children * should warn if the unmounted node was rendered by another copy of React * passes the correct callback context +* initial mount is sync inside batchedUpdates, but task work is deferred until the end of the batch src/renderers/dom/shared/__tests__/ReactMountDestruction-test.js * should destroy a react root upon request @@ -1832,6 +1833,7 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalPerf-test.js * deduplicates lifecycle names during commit to reduce overhead * supports coroutines * supports portals +* does not schedule an extra callback if setState is called during a synchronous commit phase src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js * handles isMounted even when the initial render is deferred diff --git a/src/renderers/dom/shared/__tests__/ReactMount-test.js b/src/renderers/dom/shared/__tests__/ReactMount-test.js index 3cad1fa973..a60ba17fe4 100644 --- a/src/renderers/dom/shared/__tests__/ReactMount-test.js +++ b/src/renderers/dom/shared/__tests__/ReactMount-test.js @@ -310,4 +310,39 @@ describe('ReactMount', () => { expect(calls).toBe(5); }); + + it('initial mount is sync inside batchedUpdates, but task work is deferred until the end of the batch', () => { + var container1 = document.createElement('div'); + var container2 = document.createElement('div'); + + class Foo extends React.Component { + state = {active: false}; + componentDidMount() { + this.setState({active: true}); + } + render() { + return ( +
{this.props.children + (this.state.active ? '!' : '')}
+ ); + } + } + + ReactDOM.render(
1
, container1); + + ReactDOM.unstable_batchedUpdates(() => { + // Update. Does not flush yet. + ReactDOM.render(
2
, container1); + expect(container1.textContent).toEqual('1'); + + // Initial mount on another root. Should flush immediately. + ReactDOM.render(a, container2); + // The update did not flush yet. + expect(container1.textContent).toEqual('1'); + // The initial mount flushed, but not the update scheduled in cDU. + expect(container2.textContent).toEqual('a'); + }); + // All updates have flushed. + expect(container1.textContent).toEqual('2'); + expect(container2.textContent).toEqual('a!'); + }); }); diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 29d9d154b2..1af541ab65 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -195,6 +195,10 @@ module.exports = function( // Keeps track of whether we should should batch sync updates. let isBatchingUpdates: boolean = false; + // This is needed for the weird case where the initial mount is synchronous + // even inside batchedUpdates :( + let isUnbatchingUpdates: boolean = false; + // The next work in progress fiber that we're currently working on. let nextUnitOfWork: Fiber | null = null; let nextPriorityLevel: PriorityLevel = NoWork; @@ -759,7 +763,10 @@ module.exports = function( } } - function workLoop(deadline: Deadline | null) { + function workLoop( + minPriorityLevel: PriorityLevel, + deadline: Deadline | null, + ) { // Clear any errors. clearErrors(); @@ -771,6 +778,7 @@ module.exports = function( while ( nextUnitOfWork !== null && nextPriorityLevel !== NoWork && + nextPriorityLevel <= minPriorityLevel && nextPriorityLevel <= TaskPriority ) { nextUnitOfWork = performUnitOfWork(nextUnitOfWork); @@ -788,6 +796,7 @@ module.exports = function( nextUnitOfWork !== null && !deadlineHasExpired && nextPriorityLevel !== NoWork && + nextPriorityLevel <= minPriorityLevel && nextPriorityLevel >= HighPriority ) { if (deadline.timeRemaining() > timeHeuristicForUnitOfWork) { @@ -815,7 +824,14 @@ module.exports = function( } } - function performWork(deadline: Deadline | null) { + function performDeferredWork(deadline: Deadline) { + performWork(OffscreenPriority, deadline); + } + + function performWork( + minPriorityLevel: PriorityLevel, + deadline: Deadline | null, + ) { if (__DEV__) { startWorkLoopTimer(); } @@ -849,10 +865,16 @@ module.exports = function( priorityContextBeforeReconciliation = priorityContext; let error = null; if (__DEV__) { - error = invokeGuardedCallback(null, workLoop, null, deadline); + error = invokeGuardedCallback( + null, + workLoop, + null, + minPriorityLevel, + deadline, + ); } else { try { - workLoop(deadline); + workLoop(minPriorityLevel, deadline); } catch (e) { error = e; } @@ -903,7 +925,10 @@ module.exports = function( case TaskPriority: // We have remaining synchronous or task work. Keep performing it, // regardless of whether we're inside a callback. - continue; + if (nextPriorityLevel <= minPriorityLevel) { + continue; + } + break; case HighPriority: case LowPriority: case OffscreenPriority: @@ -914,7 +939,10 @@ module.exports = function( hasRemainingAsyncWork = true; } else { // We are inside a callback. - if (!deadlineHasExpired) { + if ( + !deadlineHasExpired && + nextPriorityLevel <= minPriorityLevel + ) { // We still have time. Keep working. continue; } @@ -939,7 +967,7 @@ module.exports = function( } // If there's remaining async work, make sure we schedule another callback. if (hasRemainingAsyncWork && !isCallbackScheduled) { - scheduleDeferredCallback(performWork); + scheduleDeferredCallback(performDeferredWork); isCallbackScheduled = true; } @@ -1264,11 +1292,34 @@ module.exports = function( if (node.tag === HostRoot) { const root: FiberRoot = (node.stateNode: any); scheduleRoot(root, priorityLevel); - if (priorityLevel === SynchronousPriority) { - performWork(null); - } else if (priorityLevel !== NoWork && !isCallbackScheduled) { - scheduleDeferredCallback(performWork); - isCallbackScheduled = true; + if (!isPerformingWork) { + switch (priorityLevel) { + case SynchronousPriority: + // Perform this update now. + if (isUnbatchingUpdates) { + // We're inside unbatchedUpdates, which is inside either + // batchedUpdates or a lifecycle. We should only flush + // synchronous work, not task work. + performWork(SynchronousPriority, null); + } else { + // Flush both synchronous and task work. + performWork(TaskPriority, null); + } + break; + case TaskPriority: + invariant( + isBatchingUpdates, + 'Task updates can only be scheduled as a nested update or ' + + 'inside batchedUpdates.', + ); + break; + default: + // Schedule a callback to perform the work later. + if (!isCallbackScheduled) { + scheduleDeferredCallback(performDeferredWork); + isCallbackScheduled = true; + } + } } } else { if (__DEV__) { @@ -1335,18 +1386,22 @@ module.exports = function( // If we're not already inside a batch, we need to flush any task work // that was created by the user-provided function. if (!isPerformingWork && !isBatchingUpdates) { - performWork(null); + performWork(TaskPriority, null); } } } function unbatchedUpdates(fn: () => A): A { + const previousIsUnbatchingUpdates = isUnbatchingUpdates; const previousIsBatchingUpdates = isBatchingUpdates; + // This is only true if we're nested inside batchedUpdates. + isUnbatchingUpdates = isBatchingUpdates; isBatchingUpdates = false; try { return fn(); } finally { isBatchingUpdates = previousIsBatchingUpdates; + isUnbatchingUpdates = previousIsUnbatchingUpdates; } } diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalPerf-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalPerf-test.js index e3ef4ddb87..c2b81d19d4 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalPerf-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalPerf-test.js @@ -487,4 +487,20 @@ describe('ReactDebugFiberPerf', () => { ReactNoop.flush(); expect(getFlameChart()).toMatchSnapshot(); }); + + it('does not schedule an extra callback if setState is called during a synchronous commit phase', () => { + class Component extends React.Component { + state = {step: 1}; + componentDidMount() { + this.setState({step: 2}); + } + render() { + return ; + } + } + ReactNoop.syncUpdates(() => { + ReactNoop.render(); + }); + expect(getFlameChart()).toMatchSnapshot(); + }); }); diff --git a/src/renderers/shared/fiber/__tests__/__snapshots__/ReactIncrementalPerf-test.js.snap b/src/renderers/shared/fiber/__tests__/__snapshots__/ReactIncrementalPerf-test.js.snap index 760598a4ce..a816615ba4 100644 --- a/src/renderers/shared/fiber/__tests__/__snapshots__/ReactIncrementalPerf-test.js.snap +++ b/src/renderers/shared/fiber/__tests__/__snapshots__/ReactIncrementalPerf-test.js.snap @@ -67,6 +67,20 @@ exports[`ReactDebugFiberPerf deduplicates lifecycle names during commit to reduc " `; +exports[`ReactDebugFiberPerf does not schedule an extra callback if setState is called during a synchronous commit phase 1`] = ` +"⛔ (React Tree Reconciliation) Warning: There were cascading updates + ⚛ Component [mount] + ⛔ (Committing Changes) Warning: Lifecycle hook scheduled a cascading update + ⚛ (Committing Host Effects: 1 Total) + ⚛ (Calling Lifecycle Methods: 1 Total) + ⛔ Component.componentDidMount Warning: Scheduled a cascading update + ⚛ Component [update] + ⛔ (Committing Changes) Warning: Caused by a cascading update in earlier commit + ⚛ (Committing Host Effects: 1 Total) + ⚛ (Calling Lifecycle Methods: 1 Total) +" +`; + exports[`ReactDebugFiberPerf does not treat setState from cWM or cWRP as cascading 1`] = ` "// Should not print a warning ⚛ (React Tree Reconciliation)