From d8ade83eb2019bffe19d71a9b1a03ad2e84b3150 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 15 Dec 2016 13:58:22 -0800 Subject: [PATCH 1/2] Use an UpdateQueue for top-level updates ...rather than mutate the pendingProps directly, which throws away any previously scheduled work. --- scripts/fiber/tests-passing.txt | 24 +- src/renderers/noop/ReactNoop.js | 6 +- .../shared/fiber/ReactFiberBeginWork.js | 13 +- .../shared/fiber/ReactFiberReconciler.js | 31 +- .../shared/fiber/ReactFiberScheduler.js | 77 ++-- .../fiber/__tests__/ReactIncremental-test.js | 1 - .../ReactIncrementalScheduling-test.js | 426 +++--------------- .../ReactCompositeComponentState-test.js | 26 +- 8 files changed, 129 insertions(+), 475 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 78ed9f2cdf..83bc0ef4bd 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1181,27 +1181,13 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js * finds no node before insertion and correct node before deletion src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js -* schedules and flushes animation work -* schedules and flushes animation work for many roots -* flushes all scheduled animation work -* flushes all scheduled animation work for many roots * schedules and flushes deferred work -* schedules and flushes deferred work for many roots -* flushes scheduled deferred work fitting within deadline -* flushes scheduled deferred work fitting within deadline for many roots -* schedules more deferred work if it runs out of time -* schedules more deferred work if it runs out of time with many roots -* flushes late animation work in a deferred callback if it wins -* flushes late animation work in a deferred callback if it wins with many roots -* flushes late animation work in an animation callback if it wins -* flushes late animation work in an animation callback if it wins with many roots -* flushes all work in a deferred callback if it wins -* flushes all work in a deferred callback if it wins with many roots -* flushes root with late deferred work in an animation callback if it wins -* flushes all roots with animation work in an animation callback if it wins -* splits deferred work on multiple roots +* schedules and flushes animation work +* searches for work on other roots once the current root completes +* schedules an animation callback when there`s leftover animation work +* schedules top-level updates in order of priority +* schedules top-level updates with same priority in order of insertion * works on deferred roots in the order they were scheduled -* handles interleaved deferred and animation work * schedules sync updates when inside componentDidMount/Update * can opt-in to deferred/animation scheduling inside componentDidMount/Update * performs Task work even after time runs out diff --git a/src/renderers/noop/ReactNoop.js b/src/renderers/noop/ReactNoop.js index 946be9f3b4..56f9208130 100644 --- a/src/renderers/noop/ReactNoop.js +++ b/src/renderers/noop/ReactNoop.js @@ -294,14 +294,16 @@ var ReactNoop = { log( ' '.repeat(depth + 1) + '~', firstUpdate && firstUpdate.partialState, - firstUpdate.callback ? 'with callback' : '' + firstUpdate.callback ? 'with callback' : '', + '[' + firstUpdate.priorityLevel + ']' ); var next; while (next = firstUpdate.next) { log( ' '.repeat(depth + 1) + '~', next.partialState, - next.callback ? 'with callback' : '' + next.callback ? 'with callback' : '', + '[' + firstUpdate.priorityLevel + ']' ); } } diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index 94799919b9..ab18045de2 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -533,12 +533,15 @@ module.exports = function( pushTopLevelContextObject(root.context, false); } - if (updateQueue) { - beginUpdateQueue(workInProgress, updateQueue, null, null, null, priorityLevel); - } + pushHostContainer(root.containerInfo); - pushHostContainer(workInProgress.stateNode.containerInfo); - reconcileChildren(current, workInProgress, pendingProps); + if (updateQueue) { + const prevState = workInProgress.memoizedState; + const state = beginUpdateQueue(workInProgress, updateQueue, null, prevState, null, priorityLevel); + const element = state.element; + reconcileChildren(current, workInProgress, element); + workInProgress.memoizedState = state; + } // A yield component is just a placeholder, we can just run through the // next one immediately. diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index 177fc674dc..8505ba1f53 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -97,7 +97,7 @@ getContextForSubtree._injectFiber(function(fiber : Fiber) { module.exports = function(config : HostConfig) : Reconciler { var { - scheduleWork, + scheduleSetState, scheduleUpdateCallback, performWithPriority, batchedUpdates, @@ -112,20 +112,11 @@ module.exports = function(config : HostConfig(config : HostConfig(config : HostConfig(config : HostConfig(config : HostConfig(config : HostConfig(config : HostConfig { ReactNoop.performAnimationWork(() => { ReactNoop.render(); }); - ReactNoop.render(); ReactNoop.flushAnimationPri(); expect(ops).toEqual(['Foo', 'Bar', 'Bar']); diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js index f1bc165d2f..c00dc89f37 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js @@ -25,6 +25,14 @@ describe('ReactIncrementalScheduling', () => { return { type: 'span', children: [], prop }; } + it('schedules and flushes deferred work', () => { + ReactNoop.render(); + expect(ReactNoop.getChildren()).toEqual([]); + + ReactNoop.flushDeferredPri(); + expect(ReactNoop.getChildren()).toEqual([span('1')]); + }); + it('schedules and flushes animation work', () => { ReactNoop.performAnimationWork(() => { ReactNoop.render(); @@ -35,341 +43,75 @@ describe('ReactIncrementalScheduling', () => { expect(ReactNoop.getChildren()).toEqual([span('1')]); }); - it('schedules and flushes animation work for many roots', () => { - ReactNoop.performAnimationWork(() => { - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'c'); - }); - expect(ReactNoop.getChildren('a')).toEqual([]); - expect(ReactNoop.getChildren('b')).toEqual([]); - expect(ReactNoop.getChildren('c')).toEqual([]); + it('searches for work on other roots once the current root completes', () => { + ReactNoop.renderToRootWithID(, 'a'); + ReactNoop.renderToRootWithID(, 'b'); + ReactNoop.renderToRootWithID(, 'c'); + + ReactNoop.flush(); - ReactNoop.flushAnimationPri(); expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); expect(ReactNoop.getChildren('b')).toEqual([span('b:1')]); expect(ReactNoop.getChildren('c')).toEqual([span('c:1')]); }); - it('flushes all scheduled animation work', () => { - ReactNoop.performAnimationWork(() => { - ReactNoop.render(); - }); - ReactNoop.performAnimationWork(() => { - ReactNoop.render(); - }); - expect(ReactNoop.getChildren()).toEqual([]); + it('schedules an animation callback when there`\s leftover animation work', () => { + class Foo extends React.Component { + state = { step: 0 }; + componentDidMount() { + ReactNoop.performAnimationWork(() => { + this.setState({ step: 2 }); + }); + this.setState({ step: 1 }); + } + render() { + return ; + } + } + ReactNoop.render(); + // Flush just enough work to mount the component, but not enough to flush + // the animation update. + ReactNoop.flushDeferredPri(25); + expect(ReactNoop.getChildren()).toEqual([span(1)]); + + // There's more animation work. A callback should have been scheduled. ReactNoop.flushAnimationPri(); - expect(ReactNoop.getChildren()).toEqual([span('2')]); + expect(ReactNoop.getChildren()).toEqual([span(2)]); }); - it('flushes all scheduled animation work for many roots', () => { + it('schedules top-level updates in order of priority', () => { + // Initial render. + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span(1)]); + + ReactNoop.render(); ReactNoop.performAnimationWork(() => { - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'c'); + ReactNoop.render(); + ReactNoop.render(); + ReactNoop.render(); }); - ReactNoop.performAnimationWork(() => { - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'c'); - }); - expect(ReactNoop.getChildren('a')).toEqual([]); - expect(ReactNoop.getChildren('b')).toEqual([]); - expect(ReactNoop.getChildren('c')).toEqual([]); - ReactNoop.flushAnimationPri(); - expect(ReactNoop.getChildren('a')).toEqual([span('a:2')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); + // The low pri update should be flushed last, even though it was scheduled + // before the animation updates. + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span(5)]); }); - it('schedules and flushes deferred work', () => { - ReactNoop.render(); - expect(ReactNoop.getChildren()).toEqual([]); + it('schedules top-level updates with same priority in order of insertion', () => { + // Initial render. + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span(1)]); - ReactNoop.flushDeferredPri(); - expect(ReactNoop.getChildren()).toEqual([span('1')]); - }); + ReactNoop.render(); + ReactNoop.render(); + ReactNoop.render(); + ReactNoop.render(); - it('schedules and flushes deferred work for many roots', () => { - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'c'); - expect(ReactNoop.getChildren('a')).toEqual([]); - expect(ReactNoop.getChildren('b')).toEqual([]); - expect(ReactNoop.getChildren('c')).toEqual([]); - - ReactNoop.flushDeferredPri(); - expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:1')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:1')]); - }); - - it('flushes scheduled deferred work fitting within deadline', () => { - ReactNoop.render(); - ReactNoop.render(); - expect(ReactNoop.getChildren()).toEqual([]); - - ReactNoop.flushDeferredPri(); - expect(ReactNoop.getChildren()).toEqual([span('2')]); - }); - - it('flushes scheduled deferred work fitting within deadline for many roots', () => { - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'c'); - ReactNoop.renderToRootWithID(, 'c'); - expect(ReactNoop.getChildren('a')).toEqual([]); - expect(ReactNoop.getChildren('b')).toEqual([]); - expect(ReactNoop.getChildren('c')).toEqual([]); - - ReactNoop.flushDeferredPri(); - expect(ReactNoop.getChildren('a')).toEqual([span('a:2')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); - }); - - it('schedules more deferred work if it runs out of time', () => { - ReactNoop.render(); - ReactNoop.render(); - expect(ReactNoop.getChildren()).toEqual([]); - - ReactNoop.flushDeferredPri(5); - expect(ReactNoop.getChildren()).toEqual([]); - - ReactNoop.flushDeferredPri(10); - expect(ReactNoop.getChildren()).toEqual([]); - - ReactNoop.flushDeferredPri(10 + 5); - expect(ReactNoop.getChildren()).toEqual([span('2')]); - }); - - it('schedules more deferred work if it runs out of time with many roots', () => { - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'c'); - ReactNoop.renderToRootWithID(, 'c'); - expect(ReactNoop.getChildren('a')).toEqual([]); - expect(ReactNoop.getChildren('b')).toEqual([]); - expect(ReactNoop.getChildren('c')).toEqual([]); - - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:2')]); - expect(ReactNoop.getChildren('b')).toEqual([]); - expect(ReactNoop.getChildren('c')).toEqual([]); - - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:2')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([]); - - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:2')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); - }); - - it('flushes late animation work in a deferred callback if it wins', () => { - // Schedule early deferred - ReactNoop.render(); - // Schedule late animation - ReactNoop.performAnimationWork(() => { - ReactNoop.render(); - }); - expect(ReactNoop.getChildren()).toEqual([]); - - // We only scheduled deferred callback so that's what we get. - // It will flush everything. - ReactNoop.flushDeferredPri(); - expect(ReactNoop.getChildren()).toEqual([span('2')]); - }); - - it('flushes late animation work in a deferred callback if it wins with many roots', () => { - // Schedule early deferred - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - // Schedule late animation - ReactNoop.performAnimationWork(() => { - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'c'); - }); - expect(ReactNoop.getChildren('a')).toEqual([]); - expect(ReactNoop.getChildren('b')).toEqual([]); - expect(ReactNoop.getChildren('c')).toEqual([]); - - // We only scheduled deferred callback so that's what we get. - // It will flush everything. - ReactNoop.flushDeferredPri(); - expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); - }); - - it('flushes late animation work in an animation callback if it wins', () => { - // Schedule early deferred - ReactNoop.render(); - // Schedule late animation - ReactNoop.performAnimationWork(() => { - ReactNoop.render(); - }); - expect(ReactNoop.getChildren()).toEqual([]); - - // Flushing animation should have flushed the animation. - ReactNoop.flushAnimationPri(); - expect(ReactNoop.getChildren()).toEqual([span('2')]); - }); - - it('flushes late animation work in an animation callback if it wins with many roots', () => { - // Schedule early deferred - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - // Schedule late animation - ReactNoop.performAnimationWork(() => { - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'c'); - }); - expect(ReactNoop.getChildren('a')).toEqual([]); - expect(ReactNoop.getChildren('b')).toEqual([]); - expect(ReactNoop.getChildren('c')).toEqual([]); - - // Flushing animation should have flushed the animation. - ReactNoop.flushAnimationPri(); - expect(ReactNoop.getChildren('a')).toEqual([]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); - - ReactNoop.flushDeferredPri(); - expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); - }); - - it('flushes all work in a deferred callback if it wins', () => { - // Schedule early animation - ReactNoop.performAnimationWork(() => { - ReactNoop.render(); - }); - // Schedule late deferred - ReactNoop.render(); - expect(ReactNoop.getChildren()).toEqual([]); - - // Flushing deferred should have flushed both early animation and late deferred work that invalidated it. - // This is not a common case, as animation should generally be flushed before deferred work. - ReactNoop.flushDeferredPri(); - expect(ReactNoop.getChildren()).toEqual([span('2')]); - }); - - it('flushes all work in a deferred callback if it wins with many roots', () => { - // Schedule early animation - ReactNoop.performAnimationWork(() => { - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - }); - // Schedule late deferred - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'c'); - expect(ReactNoop.getChildren('a')).toEqual([]); - expect(ReactNoop.getChildren('b')).toEqual([]); - expect(ReactNoop.getChildren('c')).toEqual([]); - - // Flushing deferred should have flushed both early animation and late deferred work that invalidated it. - // This is not a common case, as animation should generally be flushed before deferred work. - ReactNoop.flushDeferredPri(); - expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); - }); - - it('flushes root with late deferred work in an animation callback if it wins', () => { - // Schedule early animation - ReactNoop.performAnimationWork(() => { - ReactNoop.render(); - }); - // Schedule late deferred - ReactNoop.render(); - expect(ReactNoop.getChildren()).toEqual([]); - - // Flushing animation work flushes everything on this root. - ReactNoop.flushAnimationPri(); - expect(ReactNoop.getChildren()).toEqual([span('2')]); - }); - - it('flushes all roots with animation work in an animation callback if it wins', () => { - // Schedule early animation - ReactNoop.performAnimationWork(() => { - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - }); - // Schedule late deferred - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'c'); - expect(ReactNoop.getChildren('a')).toEqual([]); - expect(ReactNoop.getChildren('b')).toEqual([]); - expect(ReactNoop.getChildren('c')).toEqual([]); - - // Flushing animation work flushes all roots with animation work. - ReactNoop.flushAnimationPri(); - expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([]); - - // Flushing deferred work flushes the root with only deferred work. - ReactNoop.flushDeferredPri(); - expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); - }); - - it('splits deferred work on multiple roots', () => { - // Schedule one root - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); - - // Schedule two roots - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - // First scheduled one gets processed first - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:2')]); - expect(ReactNoop.getChildren('b')).toEqual([]); - expect(ReactNoop.getChildren('c')).toEqual(null); - // Then the second one gets processed - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:2')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual(null); - - // Schedule three roots - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'c'); - // They get processed in the order they were scheduled - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:3')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([]); - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:3')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:3')]); - expect(ReactNoop.getChildren('c')).toEqual([]); - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:3')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:3')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:3')]); - - // Schedule one root many times - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:6')]); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span(5)]); }); it('works on deferred roots in the order they were scheduled', () => { @@ -402,54 +144,6 @@ describe('ReactIncrementalScheduling', () => { expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); }); - it('handles interleaved deferred and animation work', () => { - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'c'); - ReactNoop.flush(); - expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:1')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:1')]); - - // Schedule both deferred and animation work - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.performAnimationWork(() => { - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'c'); - }); - // We're flushing deferred work - // Still, roots with animation work are handled first - ReactNoop.flushDeferredPri(15); - expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:1')]); - ReactNoop.flushDeferredPri(15); - expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); - - // More deferred and animation work just got scheduled! - ReactNoop.renderToRootWithID(, 'c'); - ReactNoop.performAnimationWork(() => { - ReactNoop.renderToRootWithID(, 'b'); - }); - // Animation is still handled first - ReactNoop.flushDeferredPri(15); - expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:3')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); - - // Finally we handle deferred root in the order it was scheduled - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:2')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:3')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:2')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:3')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:3')]); - }); - it('schedules sync updates when inside componentDidMount/Update', () => { var instance; var ops = []; diff --git a/src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js b/src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js index 781ab0132b..446fafe747 100644 --- a/src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js +++ b/src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js @@ -169,16 +169,22 @@ describe('ReactCompositeComponent-state', () => { // componentDidMount() called setState({color:'yellow'}), which is async. // The update doesn't happen until the next flush. ['componentDidMount-end', 'orange'], + ['setState-sunrise', 'orange'], + ['setState-orange', 'orange'], ]; - // The setState callbacks in componentWillMount, and the initial callback - // passed to ReactDOM.render, should be flushed right after component - // did mount: - expected.push( - ['setState-sunrise', 'orange'], // 1 - ['setState-orange', 'orange'], // 2 - ['initial-callback', 'orange'], // 3 - ['shouldComponentUpdate-currentState', 'orange'], + // In Fiber, the initial callback is not enqueued until after any work + // scheduled by lifecycles has flushed (same semantics as a regular setState + // callback outside of a batch). In Stack, the initial render is scheduled + // inside of batchedUpdates, so the callback gets flushed right after + // componentDidMount. + // TODO: We should fix this, in both Stack and Fiber, so that the behavior + // is consistent regardless of whether you're in a batch. + if (!ReactDOMFeatureFlags.useFiber) { + expected.push(['initial-callback', 'orange']); + } + + expected.push(['shouldComponentUpdate-currentState', 'orange'], ['shouldComponentUpdate-nextState', 'yellow'], ['componentWillUpdate-currentState', 'orange'], ['componentWillUpdate-nextState', 'yellow'], @@ -188,6 +194,10 @@ describe('ReactCompositeComponent-state', () => { ['setState-yellow', 'yellow'], ); + if (ReactDOMFeatureFlags.useFiber) { + expected.push(['initial-callback', 'yellow']); + } + expected.push( ['componentWillReceiveProps-start', 'yellow'], // setState({color:'green'}) only enqueues a pending state. From a9365b97e54c56616cd338efa2c1781f2637beb4 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 16 Dec 2016 18:31:39 -0800 Subject: [PATCH 2/2] Remove unmountContainer Instead, renderers should pass null to updateContainer. --- scripts/fiber/tests-failing.txt | 1 - scripts/fiber/tests-passing.txt | 1 + src/renderers/art/ReactARTFiber.js | 6 +- src/renderers/dom/fiber/ReactDOMFiber.js | 14 ++--- src/renderers/native/ReactNativeFiber.js | 7 ++- src/renderers/noop/ReactNoop.js | 7 ++- .../shared/fiber/ReactFiberReconciler.js | 39 ++++++------ .../shared/fiber/ReactFiberScheduler.js | 9 ++- .../shared/fiber/ReactFiberUpdateQueue.js | 61 +++++++++++++++++-- .../ReactIncrementalErrorHandling-test.js | 1 + 10 files changed, 100 insertions(+), 46 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index da68b422e9..dd4907f9b9 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -90,7 +90,6 @@ src/renderers/shared/shared/__tests__/ReactUpdates-test.js * throws in setState if the update callback is not a function * throws in replaceState if the update callback is not a function * throws in forceUpdate if the update callback is not a function -* unmounts and remounts a root in the same batch src/renderers/shared/shared/__tests__/refs-test.js * Should increase refs with an increase in divs diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 83bc0ef4bd..6cd7643feb 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1550,6 +1550,7 @@ src/renderers/shared/shared/__tests__/ReactUpdates-test.js * does not update one component twice in a batch (#2410) * does not update one component twice in a batch (#6371) * unstable_batchedUpdates should return value from a callback +* unmounts and remounts a root in the same batch src/renderers/shared/shared/__tests__/refs-destruction-test.js * should remove refs when destroying the parent diff --git a/src/renderers/art/ReactARTFiber.js b/src/renderers/art/ReactARTFiber.js index ffa9cc419c..ede8d96b8c 100644 --- a/src/renderers/art/ReactARTFiber.js +++ b/src/renderers/art/ReactARTFiber.js @@ -356,7 +356,11 @@ class Surface extends Component { } componentWillUnmount() { - ARTRenderer.unmountContainer(this._mountNode); + ARTRenderer.updateContainer( + null, + this._mountNode, + this, + ); } render() { diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index 19c29643f3..fccc2e0655 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -191,7 +191,7 @@ function warnAboutUnstableUse() { warned = true; } -function renderSubtreeIntoContainer(parentComponent : ?ReactComponent, element : ReactElement, containerNode : DOMContainerElement | Document, callback: ?Function) { +function renderSubtreeIntoContainer(parentComponent : ?ReactComponent, children : ReactNodeList, containerNode : DOMContainerElement | Document, callback: ?Function) { let container : DOMContainerElement = containerNode.nodeType === DOCUMENT_NODE ? (containerNode : any).documentElement : (containerNode : any); let root; @@ -200,9 +200,9 @@ function renderSubtreeIntoContainer(parentComponent : ?ReactComponent { container._reactRootContainer = null; - DOMRenderer.unmountContainer(root); - } + }); }, findDOMNode: findDOMNode, diff --git a/src/renderers/native/ReactNativeFiber.js b/src/renderers/native/ReactNativeFiber.js index d11438a114..7595fa6b74 100644 --- a/src/renderers/native/ReactNativeFiber.js +++ b/src/renderers/native/ReactNativeFiber.js @@ -326,7 +326,7 @@ const NativeRenderer = ReactFiberReconciler({ // But creates an additional child Fiber for raw text children. // No additional native views are created though. // It's not clear to me which is better so I'm deferring for now. - // More context @ github.com/facebook/react/pull/8560#discussion_r92111303 + // More context @ github.com/facebook/react/pull/8560#discussion_r92111303 return false; }, @@ -372,8 +372,9 @@ const ReactNative = { const root = roots.get(containerTag); if (root) { // TODO: Is it safe to reset this now or should I wait since this unmount could be deferred? - roots.delete(containerTag); - NativeRenderer.unmountContainer(root); + NativeRenderer.updateContainer(null, root, null, () => { + roots.delete(containerTag); + }); } }, diff --git a/src/renderers/noop/ReactNoop.js b/src/renderers/noop/ReactNoop.js index 56f9208130..6d887d89c8 100644 --- a/src/renderers/noop/ReactNoop.js +++ b/src/renderers/noop/ReactNoop.js @@ -188,10 +188,11 @@ var ReactNoop = { unmountRootWithID(rootID : string) { const root = roots.get(rootID); - roots.delete(rootID); - rootContainers.delete(rootID); if (root) { - NoopRenderer.unmountContainer(root); + NoopRenderer.updateContainer(null, root, null, () => { + roots.delete(rootID); + rootContainers.delete(rootID); + }); } }, diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index 8505ba1f53..ac34ef9a95 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -15,6 +15,7 @@ import type { Fiber } from 'ReactFiber'; import type { FiberRoot } from 'ReactFiberRoot'; import type { PriorityLevel } from 'ReactPriorityLevel'; +import type { ReactNodeList } from 'ReactTypes'; var { findCurrentUnmaskedContext, @@ -69,9 +70,8 @@ export type HostConfig = { }; export type Reconciler = { - mountContainer(element : ReactElement, containerInfo : C, parentComponent : ?ReactComponent) : OpaqueNode, - updateContainer(element : ReactElement, container : OpaqueNode, parentComponent : ?ReactComponent) : void, - unmountContainer(container : OpaqueNode) : void, + mountContainer(element : ReactNodeList, containerInfo : C, parentComponent : ?ReactComponent) : OpaqueNode, + updateContainer(element : ReactNodeList, container : OpaqueNode, parentComponent : ?ReactComponent) : void, performWithPriority(priorityLevel : PriorityLevel, fn : Function) : void, /* eslint-disable no-undef */ // FIXME: ESLint complains about type parameter @@ -97,7 +97,7 @@ getContextForSubtree._injectFiber(function(fiber : Fiber) { module.exports = function(config : HostConfig) : Reconciler { var { - scheduleSetState, + scheduleTopLevelSetState, scheduleUpdateCallback, performWithPriority, batchedUpdates, @@ -107,12 +107,12 @@ module.exports = function(config : HostConfig, containerInfo : C, parentComponent : ?ReactComponent, callback: ?Function) : OpaqueNode { + mountContainer(element : ReactNodeList, containerInfo : C, parentComponent : ?ReactComponent, callback: ?Function) : OpaqueNode { const context = getContextForSubtree(parentComponent); const root = createFiberRoot(containerInfo, context); const current = root.current; - scheduleSetState(current, { element }); + scheduleTopLevelSetState(current, { element }); if (callback) { scheduleUpdateCallback(current, callback); } @@ -127,32 +127,27 @@ module.exports = function(config : HostConfig, container : OpaqueNode, parentComponent : ?ReactComponent, callback: ?Function) : void { + updateContainer(element : ReactNodeList, container : OpaqueNode, parentComponent : ?ReactComponent, callback: ?Function) : void { // TODO: If this is a nested container, this won't be the root. const root : FiberRoot = (container.stateNode : any); const current = root.current; root.pendingContext = getContextForSubtree(parentComponent); - scheduleSetState(current, { element }); + scheduleTopLevelSetState(current, { element }); if (callback) { scheduleUpdateCallback(current, callback); } - if (__DEV__ && ReactFiberInstrumentation.debugTool) { - ReactFiberInstrumentation.debugTool.onUpdateContainer(root); - } - }, - - unmountContainer(container : OpaqueNode) : void { - // TODO: If this is a nested container, this won't be the root. - const root : FiberRoot = (container.stateNode : any); - const current = root.current; - - scheduleSetState(current, { element: [] }); - - if (__DEV__ && ReactFiberInstrumentation.debugTool) { - ReactFiberInstrumentation.debugTool.onUnmountContainer(root); + if (__DEV__) { + if (ReactFiberInstrumentation.debugTool) { + if (element === null) { + ReactFiberInstrumentation.debugTool.onUpdateContainer(root); + } else { + // This is an unmount + ReactFiberInstrumentation.debugTool.onUnmountContainer(root); + } + } } }, diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 03ce6685dd..356c25422a 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -59,6 +59,7 @@ var { addReplaceUpdate, addForceUpdate, addCallback, + addTopLevelUpdate, } = require('ReactFiberUpdateQueue'); var { @@ -1058,6 +1059,12 @@ module.exports = function(config : HostConfig(config : HostConfig, + priorityLevel : PriorityLevel +) : void { + const isTopLevelUnmount = partialState === null; + + const update = { + priorityLevel, + partialState, + callback: null, + isReplace: false, + isForced: false, + isTopLevelUnmount, + next: null, + }; + const update2 = insertUpdate(fiber, update); + + if (isTopLevelUnmount) { + // Drop all updates that are lower-priority, so that the tree is not + // remounted. We need to do this for both queues. + const queue1 = fiber.updateQueue; + const queue2 = fiber.alternate && fiber.alternate.updateQueue; + + if (queue1 && update.next) { + update.next = null; + queue1.last = update; + } + if (queue2 && update2 && update2.next) { + update2.next = null; + queue2.last = update; + } + } +} +exports.addTopLevelUpdate = addTopLevelUpdate; + function getStateFromUpdate(update, instance, prevState, props) { const partialState = update.partialState; if (typeof partialState === 'function') { @@ -412,12 +459,14 @@ function beginUpdateQueue( if (update.isForced) { queue.hasForceUpdate = true; } - if (update.callback) { + // Second condition ignores top-level unmount callbacks if they are not the + // last update in the queue, since a subsequent update will cause a remount. + if (update.callback && !(update.isTopLevelUnmount && update.next)) { + const callbackUpdate = cloneUpdate(update); if (callbackList && callbackList.last) { - callbackList.last.next = update; - callbackList.last = update; + callbackList.last.next = callbackUpdate; + callbackList.last = callbackUpdate; } else { - const callbackUpdate = cloneUpdate(update); callbackList = { first: callbackUpdate, last: callbackUpdate, diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js index 3ee13eaa63..94a163a80d 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js @@ -654,6 +654,7 @@ describe('ReactIncrementalErrorHandling', () => { ReactNoop.unmountRootWithID('d'); ReactNoop.unmountRootWithID('e'); ReactNoop.unmountRootWithID('f'); + ReactNoop.flush(); expect(ReactNoop.getChildren('a')).toEqual(null); expect(ReactNoop.getChildren('b')).toEqual(null); expect(ReactNoop.getChildren('c')).toEqual(null);