From 8e5f12ca6c18fb48ea71cba6bfa9bd7cf499ccb6 Mon Sep 17 00:00:00 2001
From: Andrew Clark <acdlite@me.com>
Date: Mon, 26 Feb 2018 17:50:07 -0800
Subject: [PATCH] Fixes bug when initial mount of a host component is `hidden`
 (#12294)

`oldProps` was null. This went uncaught by the unit tests because
ReactNoop did not use `oldProps` in either `prepareUpdate` or
`completeUpdate`. I added some invariants so we don't regress in
the future.
---
 packages/react-noop-renderer/src/ReactNoop.js      |  9 +++++++++
 .../react-reconciler/src/ReactFiberBeginWork.js    | 14 +++++++++++++-
 .../ReactIncrementalSideEffects-test.internal.js   |  4 ++--
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/packages/react-noop-renderer/src/ReactNoop.js b/packages/react-noop-renderer/src/ReactNoop.js
index 81ac5bc4ca50d..852b6250626d5 100644
--- a/packages/react-noop-renderer/src/ReactNoop.js
+++ b/packages/react-noop-renderer/src/ReactNoop.js
@@ -130,6 +130,12 @@ let SharedHostConfig = {
     oldProps: Props,
     newProps: Props,
   ): null | {} {
+    if (oldProps === null) {
+      throw new Error('Should have old props');
+    }
+    if (newProps === null) {
+      throw new Error('Should have old props');
+    }
     return UPDATE_SIGNAL;
   },
 
@@ -196,6 +202,9 @@ const NoopRenderer = ReactFiberReconciler({
       oldProps: Props,
       newProps: Props,
     ): void {
+      if (oldProps === null) {
+        throw new Error('Should have old props');
+      }
       instance.prop = newProps.prop;
     },
 
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js
index 73eb2ef47ee2e..a5197b32025fd 100644
--- a/packages/react-reconciler/src/ReactFiberBeginWork.js
+++ b/packages/react-reconciler/src/ReactFiberBeginWork.js
@@ -474,7 +474,18 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
       // Normally we can bail out on props equality but if context has changed
       // we don't do the bailout and we have to reuse existing props instead.
     } else if (memoizedProps === nextProps) {
-      return bailoutOnAlreadyFinishedWork(current, workInProgress);
+      const isHidden =
+        workInProgress.mode & AsyncMode &&
+        shouldDeprioritizeSubtree(type, nextProps);
+      if (isHidden) {
+        // Before bailing out, make sure we've deprioritized a hidden component.
+        workInProgress.expirationTime = Never;
+      }
+      if (!isHidden || renderExpirationTime !== Never) {
+        return bailoutOnAlreadyFinishedWork(current, workInProgress);
+      }
+      // If we're rendering a hidden node at hidden priority, don't bailout. The
+      // parent is complete, but the children may not be.
     }
 
     let nextChildren = nextProps.children;
@@ -503,6 +514,7 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
       // Down-prioritize the children.
       workInProgress.expirationTime = Never;
       // Bailout and come back to this fiber later.
+      workInProgress.memoizedProps = nextProps;
       return null;
     }
 
diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js
index 226b4bb49778a..d51115b301445 100644
--- a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js
+++ b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js
@@ -758,7 +758,7 @@ describe('ReactIncrementalSideEffects', () => {
     // 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 + 5);
+    ReactNoop.flushDeferredPri(35);
     expect(ReactNoop.getChildren()).toEqual([
       div(
         // Updated.
@@ -790,7 +790,7 @@ describe('ReactIncrementalSideEffects', () => {
       ),
     ]);
 
-    expect(ops).toEqual(['Bar', 'Bar']);
+    expect(ops).toEqual(['Bar', '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