From db47031e635faf17d4ed44dfc72779154d2c86ff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= <sebastian@calyptus.eu>
Date: Mon, 26 Feb 2018 23:34:53 -0800
Subject: [PATCH] [Persistent] Finalize children after we've actually inserted
 them (#12300)

The order of this was wrong. We also unconditionally mark for updates so
killed that unused branch.
---
 .../src/__mocks__/FabricUIManager.js          | 28 ++++++++++++-------
 .../__tests__/ReactFabric-test.internal.js    | 25 +++++++++++++++++
 .../ReactFabric-test.internal.js.snap         |  7 +++++
 .../src/ReactFiberCompleteWork.js             |  6 ++--
 4 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/packages/react-native-renderer/src/__mocks__/FabricUIManager.js b/packages/react-native-renderer/src/__mocks__/FabricUIManager.js
index 9ad7d9d406..4e39e95848 100644
--- a/packages/react-native-renderer/src/__mocks__/FabricUIManager.js
+++ b/packages/react-native-renderer/src/__mocks__/FabricUIManager.js
@@ -14,18 +14,26 @@ const invariant = require('fbjs/lib/invariant');
 const roots = new Map();
 const allocatedTags = new Set();
 
+function dumpSubtree(info, indent) {
+  let out = '';
+  out += ' '.repeat(indent) + info.viewName + ' ' + JSON.stringify(info.props);
+  // eslint-disable-next-line no-for-of-loops/no-for-of-loops
+  for (const child of info.children) {
+    out += '\n' + dumpSubtree(child, indent + 2);
+  }
+  return out;
+}
+
 const RCTFabricUIManager = {
-  __dumpHierarchyForJestTestsOnly: function() {
-    function dumpSubtree(info, indent) {
-      let out = '';
-      out +=
-        ' '.repeat(indent) + info.viewName + ' ' + JSON.stringify(info.props);
-      // eslint-disable-next-line no-for-of-loops/no-for-of-loops
-      for (const child of info.children) {
-        out += '\n' + dumpSubtree(child, indent + 2);
-      }
-      return out;
+  __dumpChildSetForJestTestsOnly: function(childSet) {
+    let result = [];
+    // eslint-disable-next-line no-for-of-loops/no-for-of-loops
+    for (const child of childSet) {
+      result.push(dumpSubtree(child, 0));
     }
+    return result.join('\n');
+  },
+  __dumpHierarchyForJestTestsOnly: function() {
     let result = [];
     // eslint-disable-next-line no-for-of-loops/no-for-of-loops
     for (const [rootTag, childSet] of roots) {
diff --git a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js
index a1736cff24..f1e5b462b0 100644
--- a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js
+++ b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js
@@ -247,4 +247,29 @@ describe('ReactFabric', () => {
     ReactFabric.render(<Component />, 11);
     expect(mockArgs.length).toEqual(0);
   });
+
+  it('should call complete after inserting children', () => {
+    const View = createReactNativeComponentClass('View', () => ({
+      validAttributes: {foo: true},
+      uiViewClassName: 'View',
+    }));
+
+    const snapshots = [];
+    FabricUIManager.completeRoot.mockImplementation(function(
+      rootTag,
+      newChildSet,
+    ) {
+      snapshots.push(
+        FabricUIManager.__dumpChildSetForJestTestsOnly(newChildSet),
+      );
+    });
+
+    ReactFabric.render(
+      <View foo="a">
+        <View foo="b" />
+      </View>,
+      22,
+    );
+    expect(snapshots).toMatchSnapshot();
+  });
 });
diff --git a/packages/react-native-renderer/src/__tests__/__snapshots__/ReactFabric-test.internal.js.snap b/packages/react-native-renderer/src/__tests__/__snapshots__/ReactFabric-test.internal.js.snap
index cd6ebcb990..6e190e635d 100644
--- a/packages/react-native-renderer/src/__tests__/__snapshots__/ReactFabric-test.internal.js.snap
+++ b/packages/react-native-renderer/src/__tests__/__snapshots__/ReactFabric-test.internal.js.snap
@@ -50,6 +50,13 @@ exports[`ReactFabric renders and reorders children 2`] = `
    View {\\"title\\":\\"y\\"}"
 `;
 
+exports[`ReactFabric should call complete after inserting children 1`] = `
+Array [
+  "View {\\"foo\\":\\"a\\"}
+  View {\\"foo\\":\\"b\\"}",
+]
+`;
+
 exports[`ReactFabric should only pass props diffs to FabricUIManager.cloneNode 1`] = `
 "11
  View {\\"foo\\":\\"a\\",\\"bar\\":\\"b\\"}
diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js
index 650ec2989b..9e796dd53c 100644
--- a/packages/react-reconciler/src/ReactFiberCompleteWork.js
+++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js
@@ -281,14 +281,12 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
         } else {
           const container = portalOrRoot.containerInfo;
           let newChildSet = createContainerChildSet(container);
-          if (finalizeContainerChildren(container, newChildSet)) {
-            markUpdate(workInProgress);
-          }
-          portalOrRoot.pendingChildren = newChildSet;
           // If children might have changed, we have to add them all to the set.
           appendAllChildrenToContainer(newChildSet, workInProgress);
+          portalOrRoot.pendingChildren = newChildSet;
           // Schedule an update on the container to swap out the container.
           markUpdate(workInProgress);
+          finalizeContainerChildren(container, newChildSet);
         }
       };
       updateHostComponent = function(