From c862ba719e26764473eb9edd798429600ca746ab Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 20 Oct 2016 18:12:22 -0700 Subject: [PATCH] Fallback to current props if memoizedProps is null If work has progressed on a state update that gets resumed because of another state up, then we won't have an new pendingProps, and we also won't have any memoizedProps because it got aborted before completing. In that case, we can just fallback to the current props. I think that they can't have diverged because the only way they diverge is if there is new props. This lets us bail out on state only updates in more cases which the unit tests reflect. --- src/renderers/shared/fiber/ReactFiberClassComponent.js | 6 +++--- .../shared/fiber/__tests__/ReactIncremental-test.js | 4 ++-- .../fiber/__tests__/ReactIncrementalSideEffects-test.js | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberClassComponent.js b/src/renderers/shared/fiber/ReactFiberClassComponent.js index 33a6482f2d..7392fbb6c9 100644 --- a/src/renderers/shared/fiber/ReactFiberClassComponent.js +++ b/src/renderers/shared/fiber/ReactFiberClassComponent.js @@ -165,12 +165,12 @@ module.exports = function(scheduleUpdate : (fiber: Fiber, priorityLevel : Priori function updateClassInstance(current : Fiber, workInProgress : Fiber) : boolean { const instance = workInProgress.stateNode; - const oldProps = current.memoizedProps; + const oldProps = workInProgress.memoizedProps || current.memoizedProps; let newProps = workInProgress.pendingProps; if (!newProps) { // If there aren't any new props, then we'll reuse the memoized props. // This could be from already completed work. - newProps = workInProgress.memoizedProps; + newProps = oldProps; if (!newProps) { throw new Error('There should always be pending or memoized props.'); } @@ -199,7 +199,7 @@ module.exports = function(scheduleUpdate : (fiber: Fiber, priorityLevel : Priori if (typeof instance.shouldComponentUpdate === 'function' && !(updateQueue && updateQueue.isForced) && - workInProgress.memoizedProps !== null && + oldProps !== null && !instance.shouldComponentUpdate(newProps, newState)) { // TODO: Should this get the new props/state updated regardless? return false; diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index aa6cc24554..f15e12da28 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js @@ -625,10 +625,10 @@ describe('ReactIncremental', () => { // Normally shouldComponentUpdate->false is not enough to determine that we // can safely reuse the old props, but I think in this case it would be ok, // since it is a resume of already started work. - // Because of the above we can also not reuse the work of Bar because the + // Because of the above we can not reuse the work of Bar because the // rerender of Content will generate a new element which will mean we don't // auto-bail out from Bar. - expect(ops).toEqual(['Content', 'Bar', 'Middle']); + expect(ops).toEqual(['Bar', 'Middle']); }); diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js index 81a1e1c093..897422d227 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js @@ -598,7 +598,7 @@ describe('ReactIncrementalSideEffects', () => { ), ]); - expect(ops).toEqual(['Baz', 'Bar', 'Baz', 'Bar', 'Bar']); + expect(ops).toEqual(['Bar', 'Baz', 'Bar', 'Bar']); }); it('deprioritizes setStates that happens within a deprioritized tree', () => {