Fix host bailout for the persistent mode (#13611)

* Add regression test for persistent bailout bug * Fork more logic into updateHostComponent This is mostly copy paste. But I added a bailout only to mutation mode. Persistent mode doesn't have that props equality bailout anymore, so the Fabric test now passes. * Add failing test for persistence host minimalism * Add bailouts to the persistent host updates
2026-09-24 17:02:27 +00:00 · 2018-09-10 19:05:40 +01:00
parent 02cd9de919
commit 15252a7843
5 changed files with 354 additions and 84 deletions
@@ -237,6 +237,45 @@ describe('ReactFabric', () => {
    expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot();
  });

+  it('recreates host parents even if only children changed', () => {
+    const View = createReactNativeComponentClass('RCTView', () => ({
+      validAttributes: {title: true},
+      uiViewClassName: 'RCTView',
+    }));
+
+    const before = 'abcdefghijklmnopqrst';
+    const after = 'mxhpgwfralkeoivcstzy';
+
+    class Component extends React.Component {
+      state = {
+        chars: before,
+      };
+      render() {
+        const chars = this.state.chars.split('');
+        return (
+          <View>{chars.map(text => <View key={text} title={text} />)}</View>
+        );
+      }
+    }
+
+    const ref = React.createRef();
+    // Wrap in a host node.
+    ReactFabric.render(
+      <View>
+        <Component ref={ref} />
+      </View>,
+      11,
+    );
+    expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot();
+
+    // Call setState() so that we skip over the top-level host node.
+    // It should still get recreated despite a bailout.
+    ref.current.setState({
+      chars: after,
+    });
+    expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot();
+  });
+
  it('calls setState with no arguments', () => {
    let mockArgs;
    class Component extends React.Component {
@@ -1,5 +1,57 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP

+exports[`ReactFabric recreates host parents even if only children changed 1`] = `
+"11
+ RCTView null
+   RCTView null
+     RCTView {\\"title\\":\\"a\\"}
+     RCTView {\\"title\\":\\"b\\"}
+     RCTView {\\"title\\":\\"c\\"}
+     RCTView {\\"title\\":\\"d\\"}
+     RCTView {\\"title\\":\\"e\\"}
+     RCTView {\\"title\\":\\"f\\"}
+     RCTView {\\"title\\":\\"g\\"}
+     RCTView {\\"title\\":\\"h\\"}
+     RCTView {\\"title\\":\\"i\\"}
+     RCTView {\\"title\\":\\"j\\"}
+     RCTView {\\"title\\":\\"k\\"}
+     RCTView {\\"title\\":\\"l\\"}
+     RCTView {\\"title\\":\\"m\\"}
+     RCTView {\\"title\\":\\"n\\"}
+     RCTView {\\"title\\":\\"o\\"}
+     RCTView {\\"title\\":\\"p\\"}
+     RCTView {\\"title\\":\\"q\\"}
+     RCTView {\\"title\\":\\"r\\"}
+     RCTView {\\"title\\":\\"s\\"}
+     RCTView {\\"title\\":\\"t\\"}"
+`;
+
+exports[`ReactFabric recreates host parents even if only children changed 2`] = `
+"11
+ RCTView null
+   RCTView null
+     RCTView {\\"title\\":\\"m\\"}
+     RCTView {\\"title\\":\\"x\\"}
+     RCTView {\\"title\\":\\"h\\"}
+     RCTView {\\"title\\":\\"p\\"}
+     RCTView {\\"title\\":\\"g\\"}
+     RCTView {\\"title\\":\\"w\\"}
+     RCTView {\\"title\\":\\"f\\"}
+     RCTView {\\"title\\":\\"r\\"}
+     RCTView {\\"title\\":\\"a\\"}
+     RCTView {\\"title\\":\\"l\\"}
+     RCTView {\\"title\\":\\"k\\"}
+     RCTView {\\"title\\":\\"e\\"}
+     RCTView {\\"title\\":\\"o\\"}
+     RCTView {\\"title\\":\\"i\\"}
+     RCTView {\\"title\\":\\"v\\"}
+     RCTView {\\"title\\":\\"c\\"}
+     RCTView {\\"title\\":\\"s\\"}
+     RCTView {\\"title\\":\\"t\\"}
+     RCTView {\\"title\\":\\"z\\"}
+     RCTView {\\"title\\":\\"y\\"}"
+`;
+
 exports[`ReactFabric renders and reorders children 1`] = `
 "11
 RCTView null
@@ -46,6 +46,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
  let instanceCounter = 0;
  let hostDiffCounter = 0;
  let hostUpdateCounter = 0;
+  let hostCloneCounter = 0;

  function appendChildToContainerOrInstance(
    parentInstance: Container | Instance,
@@ -370,6 +371,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
            value: clone.id,
            enumerable: false,
          });
+          hostCloneCounter++;
          return clone;
        },

@@ -579,21 +581,33 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

    flushWithHostCounters(
      fn: () => void,
-    ): {|
-      hostDiffCounter: number,
-      hostUpdateCounter: number,
-    |} {
+    ):
+      | {|
+          hostDiffCounter: number,
+          hostUpdateCounter: number,
+        |}
+      | {|
+          hostDiffCounter: number,
+          hostCloneCounter: number,
+        |} {
      hostDiffCounter = 0;
      hostUpdateCounter = 0;
+      hostCloneCounter = 0;
      try {
        ReactNoop.flush();
-        return {
-          hostDiffCounter,
-          hostUpdateCounter,
-        };
+        return useMutation
+          ? {
+              hostDiffCounter,
+              hostUpdateCounter,
+            }
+          : {
+              hostDiffCounter,
+              hostCloneCounter,
+            };
      } finally {
        hostDiffCounter = 0;
        hostUpdateCounter = 0;
+        hostCloneCounter = 0;
      }
    },

@@ -14,10 +14,8 @@ import type {
  Instance,
  Type,
  Props,
-  UpdatePayload,
  Container,
  ChildSet,
-  HostContext,
 } from './ReactFiberHostConfig';

 import {
@@ -126,13 +124,36 @@ if (supportsMutation) {
  updateHostComponent = function(
    current: Fiber,
    workInProgress: Fiber,
-    updatePayload: null | UpdatePayload,
    type: Type,
-    oldProps: Props,
    newProps: Props,
    rootContainerInstance: Container,
-    currentHostContext: HostContext,
  ) {
+    // If we have an alternate, that means this is an update and we need to
+    // schedule a side-effect to do the updates.
+    const oldProps = current.memoizedProps;
+    if (oldProps === newProps) {
+      // In mutation mode, this is sufficient for a bailout because
+      // we won't touch this node even if children changed.
+      return;
+    }
+
+    // If we get updated because one of our children updated, we don't
+    // have newProps so we'll have to reuse them.
+    // 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.
+    const instance: Instance = workInProgress.stateNode;
+    const currentHostContext = getHostContext();
+    // TODO: Experiencing an error where oldProps is null. Suggests a host
+    // component is hitting the resume path. Figure out why. Possibly
+    // related to `hidden`.
+    const updatePayload = prepareUpdate(
+      instance,
+      type,
+      oldProps,
+      newProps,
+      rootContainerInstance,
+      currentHostContext,
+    );
    // TODO: Type this specific to this type of component.
    workInProgress.updateQueue = (updatePayload: any);
    // If the update payload indicates that there is a change or if there
@@ -211,54 +232,70 @@ if (supportsMutation) {
  updateHostComponent = function(
    current: Fiber,
    workInProgress: Fiber,
-    updatePayload: null | UpdatePayload,
    type: Type,
-    oldProps: Props,
    newProps: Props,
    rootContainerInstance: Container,
-    currentHostContext: HostContext,
  ) {
+    const currentInstance = current.stateNode;
+    const oldProps = current.memoizedProps;
    // If there are no effects associated with this node, then none of our children had any updates.
    // This guarantees that we can reuse all of them.
    const childrenUnchanged = workInProgress.firstEffect === null;
-    const currentInstance = current.stateNode;
+    if (childrenUnchanged && oldProps === newProps) {
+      // No changes, just reuse the existing instance.
+      // Note that this might release a previous clone.
+      workInProgress.stateNode = currentInstance;
+      return;
+    }
+    const recyclableInstance: Instance = workInProgress.stateNode;
+    const currentHostContext = getHostContext();
+    let updatePayload = null;
+    if (oldProps !== newProps) {
+      updatePayload = prepareUpdate(
+        recyclableInstance,
+        type,
+        oldProps,
+        newProps,
+        rootContainerInstance,
+        currentHostContext,
+      );
+    }
    if (childrenUnchanged && updatePayload === null) {
      // No changes, just reuse the existing instance.
      // Note that this might release a previous clone.
      workInProgress.stateNode = currentInstance;
-    } else {
-      let recyclableInstance = workInProgress.stateNode;
-      let newInstance = cloneInstance(
-        currentInstance,
-        updatePayload,
+      return;
+    }
+    let newInstance = cloneInstance(
+      currentInstance,
+      updatePayload,
+      type,
+      oldProps,
+      newProps,
+      workInProgress,
+      childrenUnchanged,
+      recyclableInstance,
+    );
+    if (
+      finalizeInitialChildren(
+        newInstance,
        type,
-        oldProps,
        newProps,
-        workInProgress,
-        childrenUnchanged,
-        recyclableInstance,
-      );
-      if (
-        finalizeInitialChildren(
-          newInstance,
-          type,
-          newProps,
-          rootContainerInstance,
-          currentHostContext,
-        )
-      ) {
-        markUpdate(workInProgress);
-      }
-      workInProgress.stateNode = newInstance;
-      if (childrenUnchanged) {
-        // If there are no other effects in this tree, we need to flag this node as having one.
-        // Even though we're not going to use it for anything.
-        // Otherwise parents won't know that there are new children to propagate upwards.
-        markUpdate(workInProgress);
-      } else {
-        // If children might have changed, we have to add them all to the set.
-        appendAllChildren(newInstance, workInProgress);
-      }
+        rootContainerInstance,
+        currentHostContext,
+      )
+    ) {
+      markUpdate(workInProgress);
+    }
+    workInProgress.stateNode = newInstance;
+    if (childrenUnchanged) {
+      // If there are no other effects in this tree, we need to flag this node as having one.
+      // Even though we're not going to use it for anything.
+      // Otherwise parents won't know that there are new children to propagate upwards.
+      markUpdate(workInProgress);
+    } else {
+      // If children might have changed, we have to add them all to the set.
+      appendAllChildren(newInstance, workInProgress);
    }
  };
  updateHostText = function(
@@ -290,12 +327,9 @@ if (supportsMutation) {
  updateHostComponent = function(
    current: Fiber,
    workInProgress: Fiber,
-    updatePayload: null | UpdatePayload,
    type: Type,
-    oldProps: Props,
    newProps: Props,
    rootContainerInstance: Container,
-    currentHostContext: HostContext,
  ) {
    // Noop
  };
@@ -358,39 +392,13 @@ function completeWork(
      const rootContainerInstance = getRootHostContainer();
      const type = workInProgress.type;
      if (current !== null && workInProgress.stateNode != null) {
-        // If we have an alternate, that means this is an update and we need to
-        // schedule a side-effect to do the updates.
-        const oldProps = current.memoizedProps;
-        if (oldProps !== newProps) {
-          // If we get updated because one of our children updated, we don't
-          // have newProps so we'll have to reuse them.
-          // 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.
-          const instance: Instance = workInProgress.stateNode;
-          const currentHostContext = getHostContext();
-          // TODO: Experiencing an error where oldProps is null. Suggests a host
-          // component is hitting the resume path. Figure out why. Possibly
-          // related to `hidden`.
-          const updatePayload = prepareUpdate(
-            instance,
-            type,
-            oldProps,
-            newProps,
-            rootContainerInstance,
-            currentHostContext,
-          );
-
-          updateHostComponent(
-            current,
-            workInProgress,
-            updatePayload,
-            type,
-            oldProps,
-            newProps,
-            rootContainerInstance,
-            currentHostContext,
-          );
-        }
+        updateHostComponent(
+          current,
+          workInProgress,
+          type,
+          newProps,
+          rootContainerInstance,
+        );

        if (current.ref !== workInProgress.ref) {
          markRef(workInProgress);
@@ -0,0 +1,157 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @emails react-core
+ * @jest-environment node
+ */
+
+'use strict';
+
+let React;
+let ReactNoopPersistent;
+
+describe('ReactPersistentUpdatesMinimalism', () => {
+  beforeEach(() => {
+    jest.resetModules();
+    React = require('react');
+    ReactNoopPersistent = require('react-noop-renderer/persistent');
+  });
+
+  it('should render a simple component', () => {
+    function Child() {
+      return <div>Hello World</div>;
+    }
+
+    function Parent() {
+      return <Child />;
+    }
+
+    ReactNoopPersistent.render(<Parent />);
+    expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({
+      hostDiffCounter: 0,
+      hostCloneCounter: 0,
+    });
+
+    ReactNoopPersistent.render(<Parent />);
+    expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({
+      hostDiffCounter: 1,
+      hostCloneCounter: 1,
+    });
+  });
+
+  it('should not diff referentially equal host elements', () => {
+    function Leaf(props) {
+      return (
+        <span>
+          hello
+          <b />
+          {props.name}
+        </span>
+      );
+    }
+
+    const constEl = (
+      <div>
+        <Leaf name="world" />
+      </div>
+    );
+
+    function Child() {
+      return constEl;
+    }
+
+    function Parent() {
+      return <Child />;
+    }
+
+    ReactNoopPersistent.render(<Parent />);
+    expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({
+      hostDiffCounter: 0,
+      hostCloneCounter: 0,
+    });
+
+    ReactNoopPersistent.render(<Parent />);
+    expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({
+      hostDiffCounter: 0,
+      hostCloneCounter: 0,
+    });
+  });
+
+  it('should not diff parents of setState targets', () => {
+    let childInst;
+
+    function Leaf(props) {
+      return (
+        <span>
+          hello
+          <b />
+          {props.name}
+        </span>
+      );
+    }
+
+    class Child extends React.Component {
+      state = {name: 'Batman'};
+      render() {
+        childInst = this;
+        return (
+          <div>
+            <Leaf name={this.state.name} />
+          </div>
+        );
+      }
+    }
+
+    function Parent() {
+      return (
+        <section>
+          <div>
+            <Leaf name="world" />
+            <Child />
+            <hr />
+            <Leaf name="world" />
+          </div>
+        </section>
+      );
+    }
+
+    ReactNoopPersistent.render(<Parent />);
+    expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({
+      hostDiffCounter: 0,
+      hostCloneCounter: 0,
+    });
+
+    childInst.setState({name: 'Robin'});
+    expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({
+      // section > div > Child > div
+      // section > div > Child > Leaf > span
+      // section > div > Child > Leaf > span > b
+      hostDiffCounter: 3,
+      // section
+      // section > div
+      // section > div > Child > div
+      // section > div > Child > Leaf > span
+      // section > div > Child > Leaf > span > b
+      hostCloneCounter: 5,
+    });
+
+    ReactNoopPersistent.render(<Parent />);
+    expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({
+      // Parent > section
+      // Parent > section > div
+      // Parent > section > div > Leaf > span
+      // Parent > section > div > Leaf > span > b
+      // Parent > section > div > Child > div
+      // Parent > section > div > Child > div > Leaf > span
+      // Parent > section > div > Child > div > Leaf > span > b
+      // Parent > section > div > hr
+      // Parent > section > div > Leaf > span
+      // Parent > section > div > Leaf > span > b
+      hostDiffCounter: 10,
+      hostCloneCounter: 10,
+    });
+  });
+});