[Fabric] Pass children when cloning nodes

packages/react-native-renderer/src/ReactFiberConfigFabric.js

@@ -349,9 +349,8 @@ export function cloneInstance(
   type: string,
   oldProps: Props,
   newProps: Props,
-  internalInstanceHandle: InternalInstanceHandle,
   keepChildren: boolean,
-  recyclableInstance: null | Instance,
+  newChildSet: ?ChildSet,
 ): Instance {
   const viewConfig = instance.canonical.viewConfig;
   const updatePayload = diff(oldProps, newProps, viewConfig.validAttributes);
@@ -370,12 +369,26 @@ export function cloneInstance(
       return instance;
     }
   } else {
-    if (updatePayload !== null) {
-      clone = cloneNodeWithNewChildrenAndProps(node, updatePayload);
+    // If passChildrenWhenCloningPersistedNodes is enabled, children will be non-null
+    if (newChildSet != null) {
+      if (updatePayload !== null) {
+        clone = cloneNodeWithNewChildrenAndProps(
+          node,
+          newChildSet,
+          updatePayload,
+        );
+      } else {
+        clone = cloneNodeWithNewChildren(node, newChildSet);
+      }
     } else {
-      clone = cloneNodeWithNewChildren(node);
+      if (updatePayload !== null) {
+        clone = cloneNodeWithNewChildrenAndProps(node, updatePayload);
+      } else {
+        clone = cloneNodeWithNewChildren(node);
+      }
     }
   }
+
   return {
     node: clone,
     canonical: instance.canonical,
@@ -386,7 +399,6 @@ export function cloneHiddenInstance(
   instance: Instance,
   type: string,
   props: Props,
-  internalInstanceHandle: InternalInstanceHandle,
 ): Instance {
   const viewConfig = instance.canonical.viewConfig;
   const node = instance.node;
@@ -403,7 +415,6 @@ export function cloneHiddenInstance(
 export function cloneHiddenTextInstance(
   instance: Instance,
   text: string,
-  internalInstanceHandle: InternalInstanceHandle,
 ): TextInstance {
   throw new Error('Not yet implemented.');
 }

packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/InitializeNativeFabricUIManager.js

@@ -70,12 +70,15 @@ const RCTFabricUIManager = {
       children: node.children,
     };
   }),
-  cloneNodeWithNewChildren: jest.fn(function cloneNodeWithNewChildren(node) {
+  cloneNodeWithNewChildren: jest.fn(function cloneNodeWithNewChildren(
+    node,
+    children,
+  ) {
     return {
       reactTag: node.reactTag,
       viewName: node.viewName,
       props: node.props,
-      children: [],
+      children: children ?? [],
     };
   }),
   cloneNodeWithNewProps: jest.fn(function cloneNodeWithNewProps(
@@ -91,11 +94,17 @@ const RCTFabricUIManager = {
   }),
   cloneNodeWithNewChildrenAndProps: jest.fn(
     function cloneNodeWithNewChildrenAndProps(node, newPropsDiff) {
+      let children = [];
+      if (arguments.length === 3) {
+        children = newPropsDiff;
+        newPropsDiff = arguments[2];
+      }
+
       return {
         reactTag: node.reactTag,
         viewName: node.viewName,
         props: {...node.props, ...newPropsDiff},
-        children: [],
+        children,
       };
     },
   ),

packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js

@@ -210,8 +210,13 @@ describe('ReactFabric', () => {
         11,
       );
     });
+    const argIndex = gate(flags => flags.passChildrenWhenCloningPersistedNodes)
+      ? 2
+      : 1;
     expect(
-      nativeFabricUIManager.cloneNodeWithNewChildrenAndProps.mock.calls[0][1],
+      nativeFabricUIManager.cloneNodeWithNewChildrenAndProps.mock.calls[0][
+        argIndex
+      ],
     ).toEqual({
       foo: 'b',
     });
@@ -220,6 +225,54 @@ describe('ReactFabric', () => {
     ).toMatchSnapshot();
   });
 
+  it('should not clone nodes without children when updating props', async () => {
+    const View = createReactNativeComponentClass('RCTView', () => ({
+      validAttributes: {foo: true},
+      uiViewClassName: 'RCTView',
+    }));
+
+    const Component = ({foo}) => (
+      <View>
+        <View foo={foo} />
+      </View>
+    );
+
+    await act(() => ReactFabric.render(<Component foo={true} />, 11));
+    expect(nativeFabricUIManager.completeRoot).toBeCalled();
+    jest.clearAllMocks();
+
+    await act(() => ReactFabric.render(<Component foo={false} />, 11));
+    expect(nativeFabricUIManager.cloneNode).not.toBeCalled();
+    expect(nativeFabricUIManager.cloneNodeWithNewProps).toHaveBeenCalledTimes(
+      1,
+    );
+    expect(nativeFabricUIManager.cloneNodeWithNewProps).toHaveBeenCalledWith(
+      expect.anything(),
+      {foo: false},
+    );
+
+    expect(
+      nativeFabricUIManager.cloneNodeWithNewChildren,
+    ).toHaveBeenCalledTimes(1);
+    if (gate(flags => flags.passChildrenWhenCloningPersistedNodes)) {
+      expect(
+        nativeFabricUIManager.cloneNodeWithNewChildren,
+      ).toHaveBeenCalledWith(expect.anything(), [
+        expect.objectContaining({props: {foo: false}}),
+      ]);
+      expect(nativeFabricUIManager.appendChild).not.toBeCalled();
+    } else {
+      expect(
+        nativeFabricUIManager.cloneNodeWithNewChildren,
+      ).toHaveBeenCalledWith(expect.anything());
+      expect(nativeFabricUIManager.appendChild).toHaveBeenCalledTimes(1);
+    }
+    expect(
+      nativeFabricUIManager.cloneNodeWithNewChildrenAndProps,
+    ).not.toBeCalled();
+    expect(nativeFabricUIManager.completeRoot).toBeCalled();
+  });
+
   it('should call dispatchCommand for native refs', async () => {
     const View = createReactNativeComponentClass('RCTView', () => ({
       validAttributes: {foo: true},

packages/react-noop-renderer/src/createReactNoop.js

@@ -223,9 +223,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
     type: string,
     oldProps: Props,
     newProps: Props,
-    internalInstanceHandle: Object,
     keepChildren: boolean,
-    recyclableInstance: null | Instance,
+    children: ?$ReadOnlyArray<Instance>,
   ): Instance {
     if (__DEV__) {
       checkPropStringCoercion(newProps.children, 'children');
@@ -234,7 +233,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
       id: instance.id,
       type: type,
       parent: instance.parent,
-      children: keepChildren ? instance.children : [],
+      children: keepChildren ? instance.children : children ?? [],
       text: shouldSetTextContent(type, newProps)
         ? computeText((newProps.children: any) + '', instance.context)
         : null,
@@ -740,25 +739,15 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
           instance: Instance,
           type: string,
           props: Props,
-          internalInstanceHandle: Object,
         ): Instance {
-          const clone = cloneInstance(
-            instance,
-            type,
-            props,
-            props,
-            internalInstanceHandle,
-            true,
-            null,
-          );
+          const clone = cloneInstance(instance, type, props, props, true, null);
           clone.hidden = true;
           return clone;
         },
 
         cloneHiddenTextInstance(
           instance: TextInstance,
           text: string,
-          internalInstanceHandle: Object,
         ): TextInstance {
           const clone = {
             text: instance.text,

packages/react-reconciler/src/ReactFiberCompleteWork.js

@@ -38,6 +38,7 @@ import {
   enableCache,
   enableTransitionTracing,
   enableFloat,
+  passChildrenWhenCloningPersistedNodes,
 } from 'shared/ReactFeatureFlags';
 
 import {now} from './Scheduler';
@@ -258,22 +259,21 @@ function appendAllChildren(
     // children to find all the terminal nodes.
     let node = workInProgress.child;
     while (node !== null) {
-      // eslint-disable-next-line no-labels
-      branches: if (node.tag === HostComponent) {
+      if (node.tag === HostComponent) {
         let instance = node.stateNode;
         if (needsVisibilityToggle && isHidden) {
           // This child is inside a timed out tree. Hide it.
           const props = node.memoizedProps;
           const type = node.type;
-          instance = cloneHiddenInstance(instance, type, props, node);
+          instance = cloneHiddenInstance(instance, type, props);
         }
         appendInitialChild(parent, instance);
       } else if (node.tag === HostText) {
         let instance = node.stateNode;
         if (needsVisibilityToggle && isHidden) {
           // This child is inside a timed out tree. Hide it.
           const text = node.memoizedProps;
-          instance = cloneHiddenTextInstance(instance, text, node);
+          instance = cloneHiddenTextInstance(instance, text);
         }
         appendInitialChild(parent, instance);
       } else if (node.tag === HostPortal) {
@@ -290,7 +290,12 @@ function appendAllChildren(
         if (child !== null) {
           child.return = node;
         }
-        appendAllChildren(parent, node, true, true);
+        appendAllChildren(
+          parent,
+          node,
+          /* needsVisibilityToggle */ true,
+          /* isHidden */ true,
+        );
       } else if (node.child !== null) {
         node.child.return = node;
         node = node.child;
@@ -328,21 +333,21 @@ function appendAllChildrenToContainer(
     let node = workInProgress.child;
     while (node !== null) {
       // eslint-disable-next-line no-labels
-      branches: if (node.tag === HostComponent) {
+      if (node.tag === HostComponent) {
         let instance = node.stateNode;
         if (needsVisibilityToggle && isHidden) {
           // This child is inside a timed out tree. Hide it.
           const props = node.memoizedProps;
           const type = node.type;
-          instance = cloneHiddenInstance(instance, type, props, node);
+          instance = cloneHiddenInstance(instance, type, props);
         }
         appendChildToContainerChildSet(containerChildSet, instance);
       } else if (node.tag === HostText) {
         let instance = node.stateNode;
         if (needsVisibilityToggle && isHidden) {
           // This child is inside a timed out tree. Hide it.
           const text = node.memoizedProps;
-          instance = cloneHiddenTextInstance(instance, text, node);
+          instance = cloneHiddenTextInstance(instance, text);
         }
         appendChildToContainerChildSet(containerChildSet, instance);
       } else if (node.tag === HostPortal) {
@@ -364,8 +369,8 @@ function appendAllChildrenToContainer(
         appendAllChildrenToContainer(
           containerChildSet,
           node,
-          _needsVisibilityToggle,
-          true,
+          /* needsVisibilityToggle */ _needsVisibilityToggle,
+          /* isHidden */ true,
         );
       } else if (node.child !== null) {
         node.child.return = node;
@@ -449,16 +454,27 @@ function updateHostComponent(
       workInProgress.stateNode = currentInstance;
       return;
     }
-    const recyclableInstance: Instance = workInProgress.stateNode;
     const currentHostContext = getHostContext();
+
+    let newChildSet = null;
+    if (!childrenUnchanged && passChildrenWhenCloningPersistedNodes) {
+      newChildSet = createContainerChildSet();
+      // If children might have changed, we have to add them all to the set.
+      appendAllChildrenToContainer(
+        newChildSet,
+        workInProgress,
+        /* needsVisibilityToggle */ false,
+        /* isHidden */ false,
+      );
+    }
+
     const newInstance = cloneInstance(
       currentInstance,
       type,
       oldProps,
       newProps,
-      workInProgress,
       childrenUnchanged,
-      recyclableInstance,
+      newChildSet,
     );
     if (newInstance === currentInstance) {
       // No changes, just reuse the existing instance.
@@ -481,7 +497,7 @@ function updateHostComponent(
       // 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 {
+    } else if (!passChildrenWhenCloningPersistedNodes) {
       // If children might have changed, we have to add them all to the set.
       appendAllChildren(
         newInstance,
@@ -1274,6 +1290,8 @@ function completeWork(
             currentHostContext,
             workInProgress,
           );
+          // TODO: For persistent renderers, we should pass children as part
+          // of the initial instance creation
           appendAllChildren(instance, workInProgress, false, false);
           workInProgress.stateNode = instance;
 

scripts/flow/react-native-host-hooks.js

@@ -176,9 +176,16 @@ declare var nativeFabricUIManager: {
     eventTarget: Object,
   ) => Object,
   cloneNode: (node: Object) => Object,
-  cloneNodeWithNewChildren: (node: Object) => Object,
+  cloneNodeWithNewChildren: (
+    node: Object,
+    children?: $ReadOnlyArray<Object>,
+  ) => Object,
   cloneNodeWithNewProps: (node: Object, newProps: ?Object) => Object,
-  cloneNodeWithNewChildrenAndProps: (node: Object, newProps: ?Object) => Object,
+  cloneNodeWithNewChildrenAndProps: (
+    node: Object,
+    newPropsOrChildren: ?Object | $ReadOnlyArray<Object>,
+    newProps?: ?Object,
+  ) => Object,
   appendChild: (node: Object, childNode: Object) => void,
 
   createChildSet: () => Object,