From 9c25538e13bcd3cc473b69f6cbaec64e77ddd196 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 10 Oct 2016 15:33:37 -0700 Subject: [PATCH] Fix resuming bug If we abort work but have some completed, we can bail out if the shouldComponentUpdate returns true. However, then we have a tree that is low priority. When we bailout we currently use the "current" tree in this case. I don't think this is right. I'm not sure why I did that. Similarly, when we complete we use the "current" props if we didn't have pending props to do. However, we should be using the memoized props of that tree if it is a pending work tree. Added a unit test that covers these two cases. --- .../shared/fiber/ReactFiberBeginWork.js | 13 +- .../shared/fiber/ReactFiberCompleteWork.js | 2 +- .../ReactIncrementalSideEffects-test.js | 150 +++++++++++++++++- 3 files changed, 150 insertions(+), 15 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index 50b4e0cd2c..c03d709738 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -402,17 +402,8 @@ module.exports = function(config : HostConfig, g } function bailoutOnLowPriority(current, workInProgress) { - if (current) { - // TODO: If I have started work on this node, mark it as finished, then - // return do other work, come back and hit this node... we killed that - // work. It is now in an inconsistent state. We probably need to check - // progressedChild or something. - workInProgress.child = current.child; - workInProgress.memoizedProps = current.memoizedProps; - workInProgress.output = current.output; - workInProgress.firstEffect = null; - workInProgress.lastEffect = null; - } + // TODO: What if this is currently in progress? + // How can that happen? How is this not being cloned? return null; } diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index 3dfaa7defc..4250f97bb0 100644 --- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js +++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js @@ -151,7 +151,7 @@ module.exports = function(config : HostConfig) { // TODO: Split the update API as separate for the props vs. children. // Even better would be if children weren't special cased at all tho. if (!newProps) { - newProps = oldProps; + newProps = workInProgress.memoizedProps || oldProps; } const instance : I = workInProgress.stateNode; if (prepareUpdate(instance, oldProps, newProps)) { diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js index 3347b7212f..9982a078e8 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js @@ -380,7 +380,7 @@ describe('ReactIncrementalSideEffects', () => { it('can defer side-effects and resume them later on', function() { class Bar extends React.Component { shouldComponentUpdate(nextProps) { - return this.props.idx !== nextProps; + return this.props.idx !== nextProps.idx; } render() { return ; @@ -437,10 +437,11 @@ describe('ReactIncrementalSideEffects', () => { ) ), ]); - ReactNoop.flushDeferredPri(30); + ReactNoop.render(); + ReactNoop.flush(); expect(ReactNoop.root.children).toEqual([ div( - span(2), + span(3), div( // New numbers. span(1), @@ -456,6 +457,149 @@ describe('ReactIncrementalSideEffects', () => { expect(innerSpanA).toBe(innerSpanB); }); + it('can defer side-effects and reuse them later - complex', function() { + var ops = []; + + class Bar extends React.Component { + shouldComponentUpdate(nextProps) { + return this.props.idx !== nextProps.idx; + } + render() { + ops.push('Bar'); + return ; + } + } + class Baz extends React.Component { + shouldComponentUpdate(nextProps) { + return this.props.idx !== nextProps.idx; + } + render() { + ops.push('Baz'); + return [, ]; + } + } + function Foo(props) { + ops.push('Foo'); + return ( +
+ + +
+ ); + } + ReactNoop.render(); + ReactNoop.flushDeferredPri(65); + expect(ReactNoop.root.children).toEqual([ + div( + span(0), + div(/*the spans are down-prioritized and not rendered yet*/) + ), + ]); + + expect(ops).toEqual(['Foo', 'Baz', 'Bar']); + ops = []; + + ReactNoop.render(); + ReactNoop.flushDeferredPri(70); + expect(ReactNoop.root.children).toEqual([ + div( + span(1), + div(/*still not rendered yet*/) + ), + ]); + + expect(ops).toEqual(['Foo', 'Baz', 'Bar']); + ops = []; + + ReactNoop.flush(); + expect(ReactNoop.root.children).toEqual([ + div( + span(1), + div( + // Now we had enough time to finish the spans. + span(0), + span(0), + span(0), + span(0), + span(0), + span(0) + ) + ), + ]); + + expect(ops).toEqual(['Bar', 'Baz', 'Bar', 'Bar', 'Baz', 'Bar', 'Bar']); + ops = []; + + // Now we're going to update the index but we'll only let it finish half + // way through. + ReactNoop.render(); + ReactNoop.flushDeferredPri(95); + expect(ReactNoop.root.children).toEqual([ + div( + span(2), + div( + // Still same old numbers. + span(0), + span(0), + span(0), + span(0), + span(0), + span(0) + ) + ), + ]); + + // We let it finish half way through. That means we'll have one fully + // completed Baz, one half-way completed Baz and one fully incomplete Baz. + expect(ops).toEqual(['Foo', 'Baz', 'Bar', 'Bar', 'Baz', 'Bar']); + ops = []; + + // We'll update again, without letting the new index update yet. Only half + // way through. + ReactNoop.render(); + ReactNoop.flushDeferredPri(50); + expect(ReactNoop.root.children).toEqual([ + div( + span(3), + div( + // Old numbers. + span(0), + span(0), + span(0), + span(0), + span(0), + span(0) + ) + ), + ]); + + expect(ops).toEqual(['Foo']); + ops = []; + + // We should now be able to reuse some of the work we've already done + // and replay those side-effects. + ReactNoop.flush(); + expect(ReactNoop.root.children).toEqual([ + div( + span(3), + div( + // New numbers. + span(1), + span(1), + span(1), + span(1), + span(1), + span(1) + ) + ), + ]); + + expect(ops).toEqual(['Baz', 'Bar', 'Baz', '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