From a5195102d4278cd4e2c36612afa7f1fc09b201fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Wed, 19 Oct 2016 13:32:37 -0400 Subject: [PATCH] [Fiber] Some setState related issues (#8010) * Use the memoized props/state from the workInProgress We don't want to use the "current" props/state because if we have started work, then pause and continue then we'll have the newer props/state on it already. If it is not a resume, it should be the same as current. * Deprioritize setState within a deprioritized tree Currently we flag the path to a setState as a higher priority than "offscreen". When we reconcile down this tree we bail out if it is a hidden node. However, in the case that node is already completed we don't hit that bail out path. We end up doing the work immediately which ends up committing that part of the tree at a higher priority. This ensures that we don't let a deprioritized subtree get reconciled at a higher priority. * Bump idx in unit test This proves that this number is actually deprioritized. --- .../shared/fiber/ReactFiberBeginWork.js | 41 ++++++- .../fiber/__tests__/ReactIncremental-test.js | 92 +++++++++++++++ .../ReactIncrementalSideEffects-test.js | 107 ++++++++++++++++++ 3 files changed, 234 insertions(+), 6 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index a1fc0b9986..b75fe2e67b 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -160,15 +160,24 @@ module.exports = function(config : HostConfig, s // Account for the possibly of missing pending props by falling back to the // memoized props. var props = workInProgress.pendingProps; - if (!props && current) { - props = current.memoizedProps; + if (!props) { + // If there isn't any new props, then we'll reuse the memoized props. + // This could be from already completed work. + props = workInProgress.memoizedProps; + if (!props) { + throw new Error('There should always be pending or memoized props.'); + } } + // Compute the state using the memoized state and the update queue. var updateQueue = workInProgress.updateQueue; - var previousState = current ? current.memoizedState : null; - var state = updateQueue ? - mergeUpdateQueue(updateQueue, previousState, props) : - previousState; + var previousState = workInProgress.memoizedState; + var state; + if (updateQueue) { + state = mergeUpdateQueue(updateQueue, previousState, props); + } else { + state = previousState; + } var instance = workInProgress.stateNode; if (!instance) { @@ -300,6 +309,26 @@ module.exports = function(config : HostConfig, s function bailoutOnAlreadyFinishedWork(current, workInProgress : Fiber) : ?Fiber { const priorityLevel = workInProgress.pendingWorkPriority; + if (workInProgress.tag === HostComponent && + workInProgress.memoizedProps.hidden && + workInProgress.pendingWorkPriority !== OffscreenPriority) { + // This subtree still has work, but it should be deprioritized so we need + // to bail out and not do any work yet. + // TODO: It would be better if this tree got its correct priority set + // during scheduleUpdate instead because otherwise we'll start a higher + // priority reconciliation first before we can get down here. However, + // that is a bit tricky since workInProgress and current can have + // different "hidden" settings. + let child = workInProgress.progressedChild; + while (child) { + // To ensure that this subtree gets its priority reset, the children + // need to be reset. + child.pendingWorkPriority = OffscreenPriority; + child = child.sibling; + } + return null; + } + // TODO: We should ideally be able to bail out early if the children have no // more work to do. However, since we don't have a separation of this // Fiber's priority and its children yet - we don't know without doing lots diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index 6d4db53996..aa6cc24554 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js @@ -835,4 +835,96 @@ describe('ReactIncremental', () => { ReactNoop.flush(); expect(ops).toEqual(['Foo', 'Bar', 'Baz', 'Bar', 'Baz']); }); + + it('can call sCU while resuming a partly mounted component', () => { + var ops = []; + + class Bar extends React.Component { + state = { y: 'A' }; + shouldComponentUpdate(newProps, newState) { + return this.props.x !== newProps.x || + this.state.y !== newState.y; + } + render() { + ops.push('Bar:' + this.props.x); + return ; + } + } + + function Foo(props) { + ops.push('Foo'); + return [ + , + , + , + ]; + } + + ReactNoop.render(); + ReactNoop.flushDeferredPri(30); + expect(ops).toEqual(['Foo', 'Bar:A', 'Bar:B']); + + ops = []; + + ReactNoop.render(); + ReactNoop.flushDeferredPri(40); + expect(ops).toEqual(['Foo', 'Bar:B', 'Bar:C']); + }); + + it('gets new props when setting state on a partly updated component', () => { + var ops = []; + var instances = []; + + class Bar extends React.Component { + state = { y: 'A' }; + constructor() { + super(); + instances.push(this); + } + performAction() { + this.setState({ + y: 'B', + }); + } + render() { + ops.push('Bar:' + this.props.x + '-' + this.props.step); + return ; + } + } + + function Baz() { + // This component is used as a sibling to Foo so that we can fully + // complete Foo, without committing. + ops.push('Baz'); + return
; + } + + function Foo(props) { + ops.push('Foo'); + return [ + , + , + ]; + } + + ReactNoop.render(
); + ReactNoop.flush(); + + ops = []; + + // Flush part way through with new props, fully completing the first Bar. + // However, it doesn't commit yet. + ReactNoop.render(
); + ReactNoop.flushDeferredPri(45); + expect(ops).toEqual(['Foo', 'Bar:A-1', 'Bar:B-1', 'Baz']); + + // Make an update to the same Bar. + instances[0].performAction(); + + ops = []; + + ReactNoop.flush(); + expect(ops).toEqual(['Bar:A-1', 'Baz', 'Baz']); + }); + }); diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js index 9982a078e8..81a1e1c093 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js @@ -601,6 +601,113 @@ describe('ReactIncrementalSideEffects', () => { expect(ops).toEqual(['Baz', 'Bar', 'Baz', 'Bar', 'Bar']); }); + it('deprioritizes setStates that happens within a deprioritized tree', () => { + var ops = []; + + var barInstances = []; + + class Bar extends React.Component { + constructor() { + super(); + this.state = { active: false }; + barInstances.push(this); + } + activate() { + this.setState({ active: true }); + } + render() { + ops.push('Bar'); + return ; + } + } + function Foo(props) { + ops.push('Foo'); + return ( +
+ + +
+ ); + } + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.root.children).toEqual([ + div( + span(0), + div( + span(0), + span(0), + span(0) + ) + ), + ]); + + expect(ops).toEqual(['Foo', 'Bar', 'Bar', 'Bar']); + + ops = []; + + ReactNoop.render(); + ReactNoop.flushDeferredPri(70); + expect(ReactNoop.root.children).toEqual([ + div( + // Updated. + span(1), + div( + // Still not updated. + span(0), + span(0), + span(0) + ) + ), + ]); + + expect(ops).toEqual(['Foo', 'Bar', 'Bar']); + ops = []; + + barInstances[0].activate(); + + // This should not be enough time to render the content of all the hidden + // items. Including the set state since that is deprioritized. + // TODO: The cycles it takes to do this could be lowered with further + // optimizations. + ReactNoop.flushDeferredPri(60); + expect(ReactNoop.root.children).toEqual([ + div( + // Updated. + span(1), + div( + // Still not updated. + span(0), + span(0), + span(0) + ) + ), + ]); + + expect(ops).toEqual(['Bar']); + ops = []; + + // However, once we render fully, we will have enough time to finish it all + // at once. + ReactNoop.flush(); + expect(ReactNoop.root.children).toEqual([ + div( + span(1), + div( + // Now we had enough time to finish the spans. + span('X'), + span(1), + span(1), + ) + ), + ]); + + expect(ops).toEqual(['Bar', 'Bar']); + }); // TODO: Test that side-effects are not cut off when a work in progress node // 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?