[Fabric] Pass children when cloning (#27458)

## Summary Currently when cloning nodes in Fabric, we reset a node's children on each clone, and then repeatedly call appendChild to restore the previous list of children (even if it was quasi-identical to before). This causes unnecessary invalidation of the layout state in Fabric's ShadowNode data (which in turn may require additional yoga clones) and extra JSI calls. This PR adds a feature flag to pass in the children as part of the clone call, so Fabric always has a complete view of the node that's being mutated. This feature flag requires matching changes in the react-native repo: facebook/react-native#39817 ## How did you test this change? Unit test added demonstrates the new behaviour ``` yarn test -r www-modern ReactFabric-test yarn test ReactFabric-test.internal ``` Tested a manual sync into React Native and verified core surfaces render correctly. DiffTrain build for commit 151e75a.
facebook · Oct 10, 2023 · a19627e · a19627e
1 parent c6c6388
commit a19627e
Show file tree

Hide file tree

Showing 11 changed files with 241 additions and 126 deletions.
diff --git a/...ook-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-dev.js b/...ook-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-dev.js
@@ -7,7 +7,7 @@
  * @noflow
  * @nolint
  * @preventMunge
- * @generated SignedSource<<99f1fded76af1fea6984fd8b08571351>>
+ * @generated SignedSource<<73b93e8e6858b35c538fb675ac42495a>>
  */
 
 'use strict';
@@ -16335,7 +16335,8 @@ function completeWork(current, workInProgress, renderLanes) {
             _rootContainerInstance,
             _currentHostContext,
             workInProgress
-          );
+          ); // TODO: For persistent renderers, we should pass children as part
+          // of the initial instance creation
 
           appendAllChildren(_instance3, workInProgress);
           workInProgress.stateNode = _instance3; // Certain renderers require commit-time effects for initial mount.
@@ -24772,7 +24773,7 @@ function createFiberRoot(
   return root;
 }
 
-var ReactVersion = "18.3.0-canary-dddfe6882-20231005";
+var ReactVersion = "18.3.0-canary-151e75a12-20231010";
 
 // Might add PROFILE later.
 

diff --git a/...ok-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-prod.js b/...ok-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-prod.js
@@ -8978,7 +8978,7 @@ var devToolsConfig$jscomp$inline_998 = {
     throw Error("TestRenderer does not support findFiberByHostInstance()");
   },
   bundleType: 0,
-  version: "18.3.0-canary-dddfe6882-20231005",
+  version: "18.3.0-canary-151e75a12-20231010",
   rendererPackageName: "react-test-renderer"
 };
 var internals$jscomp$inline_1191 = {
@@ -9009,7 +9009,7 @@ var internals$jscomp$inline_1191 = {
   scheduleRoot: null,
   setRefreshHandler: null,
   getCurrentFiber: null,
-  reconcilerVersion: "18.3.0-canary-dddfe6882-20231005"
+  reconcilerVersion: "18.3.0-canary-151e75a12-20231010"
 };
 if ("undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__) {
   var hook$jscomp$inline_1192 = __REACT_DEVTOOLS_GLOBAL_HOOK__;

diff --git a/...source/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-profiling.js b/...source/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-profiling.js
@@ -9404,7 +9404,7 @@ var devToolsConfig$jscomp$inline_1040 = {
     throw Error("TestRenderer does not support findFiberByHostInstance()");
   },
   bundleType: 0,
-  version: "18.3.0-canary-dddfe6882-20231005",
+  version: "18.3.0-canary-151e75a12-20231010",
   rendererPackageName: "react-test-renderer"
 };
 var internals$jscomp$inline_1232 = {
@@ -9435,7 +9435,7 @@ var internals$jscomp$inline_1232 = {
   scheduleRoot: null,
   setRefreshHandler: null,
   getCurrentFiber: null,
-  reconcilerVersion: "18.3.0-canary-dddfe6882-20231005"
+  reconcilerVersion: "18.3.0-canary-151e75a12-20231010"
 };
 if ("undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__) {
   var hook$jscomp$inline_1233 = __REACT_DEVTOOLS_GLOBAL_HOOK__;

diff --git a/compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-dev.js b/compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-dev.js
@@ -27,7 +27,7 @@ if (
 }
           "use strict";
 
-var ReactVersion = "18.3.0-canary-dddfe6882-20231005";
+var ReactVersion = "18.3.0-canary-151e75a12-20231010";
 
 // ATTENTION
 // When adding new symbols to this file,

diff --git a/compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-prod.js b/compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-prod.js
@@ -616,4 +616,4 @@ exports.useSyncExternalStore = function (
 exports.useTransition = function () {
   return ReactCurrentDispatcher.current.useTransition();
 };
-exports.version = "18.3.0-canary-dddfe6882-20231005";
+exports.version = "18.3.0-canary-151e75a12-20231010";
diff --git a/compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-profiling.js b/compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-profiling.js
@@ -619,7 +619,7 @@ exports.useSyncExternalStore = function (
 exports.useTransition = function () {
   return ReactCurrentDispatcher.current.useTransition();
 };
-exports.version = "18.3.0-canary-dddfe6882-20231005";
+exports.version = "18.3.0-canary-151e75a12-20231010";
 
           /* global __REACT_DEVTOOLS_GLOBAL_HOOK__ */
 if (

diff --git a/compiled-rn/facebook-fbsource/xplat/js/react-native-github/Libraries/Renderer/REVISION b/compiled-rn/facebook-fbsource/xplat/js/react-native-github/Libraries/Renderer/REVISION
@@ -1 +1 @@
-dddfe688206dafa5646550d351eb9a8e9c53654a
+151e75a128d0fd436dce365335b96c5686f704d4
diff --git a/...rce/xplat/js/react-native-github/Libraries/Renderer/implementations/ReactFabric-dev.fb.js b/...rce/xplat/js/react-native-github/Libraries/Renderer/implementations/ReactFabric-dev.fb.js
@@ -7,7 +7,7 @@
  * @noflow
  * @nolint
  * @preventMunge
- * @generated SignedSource<<d547d784dfd2ff8ab9a6adadb432b1cc>>
+ * @generated SignedSource<<f5489af768a0de079885e316e09faadd>>
  */
 
 'use strict';
@@ -3180,7 +3180,9 @@ var enableUseRefAccessWarning = dynamicFlags.enableUseRefAccessWarning,
     dynamicFlags.enableDeferRootSchedulingToMicrotask,
   alwaysThrottleRetries = dynamicFlags.alwaysThrottleRetries,
   useMicrotasksForSchedulingInFabric =
-    dynamicFlags.useMicrotasksForSchedulingInFabric; // The rest of the flags are static for better dead code elimination.
+    dynamicFlags.useMicrotasksForSchedulingInFabric,
+  passChildrenWhenCloningPersistedNodes =
+    dynamicFlags.passChildrenWhenCloningPersistedNodes; // The rest of the flags are static for better dead code elimination.
 var enableSchedulingProfiler = true;
 var enableProfilerTimer = true;
 var enableProfilerCommitHooks = true;
@@ -5007,9 +5009,8 @@ function cloneInstance(
   type,
   oldProps,
   newProps,
-  internalInstanceHandle,
   keepChildren,
-  recyclableInstance
+  newChildSet
 ) {
   var viewConfig = instance.canonical.viewConfig;
   var updatePayload = diff(oldProps, newProps, viewConfig.validAttributes); // TODO: If the event handlers have changed, we need to update the current props
@@ -5028,10 +5029,23 @@ 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);
+      }
     }
   }
 
@@ -5040,7 +5054,7 @@ function cloneInstance(
     canonical: instance.canonical
   };
 }
-function cloneHiddenInstance(instance, type, props, internalInstanceHandle) {
+function cloneHiddenInstance(instance, type, props) {
   var viewConfig = instance.canonical.viewConfig;
   var node = instance.node;
   var updatePayload = create(
@@ -5056,19 +5070,29 @@ function cloneHiddenInstance(instance, type, props, internalInstanceHandle) {
     canonical: instance.canonical
   };
 }
-function cloneHiddenTextInstance(instance, text, internalInstanceHandle) {
+function cloneHiddenTextInstance(instance, text) {
   throw new Error("Not yet implemented.");
 }
-function createContainerChildSet(container) {
-  return createChildNodeSet(container);
+function createContainerChildSet() {
+  if (passChildrenWhenCloningPersistedNodes) {
+    return [];
+  } else {
+    return createChildNodeSet();
+  }
 }
 function appendChildToContainerChildSet(childSet, child) {
-  appendChildNodeToSet(childSet, child.node);
+  if (passChildrenWhenCloningPersistedNodes) {
+    childSet.push(child.node);
+  } else {
+    appendChildNodeToSet(childSet, child.node);
+  }
 }
 function finalizeContainerChildren(container, newChildren) {
   completeRoot(container, newChildren);
 }
-function replaceContainerChildren(container, newChildren) {}
+function replaceContainerChildren(container, newChildren) {
+  // Noop - children will be replaced in finalizeContainerChildren
+}
 function preloadInstance(type, props) {
   return true;
 }
@@ -18734,7 +18758,6 @@ function appendAllChildren(
     var _node = workInProgress.child;
 
     while (_node !== null) {
-      // eslint-disable-next-line no-labels
       if (_node.tag === HostComponent) {
         var instance = _node.stateNode;
 
@@ -18764,7 +18787,14 @@ function appendAllChildren(
           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;
@@ -18836,7 +18866,9 @@ function appendAllChildrenToContainer(
         appendAllChildrenToContainer(
           containerChildSet,
           node,
+          /* needsVisibilityToggle */
           _needsVisibilityToggle,
+          /* isHidden */
           true
         );
       } else if (node.child !== null) {
@@ -18874,9 +18906,16 @@ function updateHostContainer(current, workInProgress) {
     if (childrenUnchanged);
     else {
       var container = portalOrRoot.containerInfo;
-      var newChildSet = createContainerChildSet(container); // If children might have changed, we have to add them all to the set.
+      var newChildSet = createContainerChildSet(); // If children might have changed, we have to add them all to the set.
 
-      appendAllChildrenToContainer(newChildSet, workInProgress, false, false);
+      appendAllChildrenToContainer(
+        newChildSet,
+        workInProgress,
+        /* needsVisibilityToggle */
+        false,
+        /* isHidden */
+        false
+      );
       portalOrRoot.pendingChildren = newChildSet; // Schedule an update on the container to swap out the container.
 
       markUpdate(workInProgress);
@@ -18905,22 +18944,38 @@ function updateHostComponent(
       workInProgress.stateNode = currentInstance;
       return;
     }
+
     getHostContext();
+    var 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
+      );
+    }
+
     var newInstance = cloneInstance(
       currentInstance,
       type,
       _oldProps,
       newProps,
-      workInProgress,
-      childrenUnchanged
+      childrenUnchanged,
+      newChildSet
     );
 
     if (newInstance === currentInstance) {
       // No changes, just reuse the existing instance.
       // Note that this might release a previous clone.
       workInProgress.stateNode = currentInstance;
       return;
-    }
+    } // Certain renderers require commit-time effects for initial mount.
 
     workInProgress.stateNode = newInstance;
 
@@ -18929,9 +18984,16 @@ 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, workInProgress, false, false);
+      appendAllChildren(
+        newInstance,
+        workInProgress,
+        /* needsVisibilityToggle */
+        false,
+        /* isHidden */
+        false
+      );
     }
   }
 } // This function must be called at the very end of the complete phase, because
@@ -19399,7 +19461,8 @@ function completeWork(current, workInProgress, renderLanes) {
             _rootContainerInstance,
             _currentHostContext,
             workInProgress
-          );
+          ); // TODO: For persistent renderers, we should pass children as part
+          // of the initial instance creation
 
           appendAllChildren(_instance3, workInProgress, false, false);
           workInProgress.stateNode = _instance3; // Certain renderers require commit-time effects for initial mount.
@@ -21092,9 +21155,7 @@ function detachFiberAfterEffects(fiber) {
 }
 
 function emptyPortalContainer(current) {
-  var portal = current.stateNode;
-  var containerInfo = portal.containerInfo;
-  createContainerChildSet(containerInfo);
+  createContainerChildSet();
 }
 
 function commitPlacement(finishedWork) {
@@ -21167,7 +21228,7 @@ function commitDeletionEffectsOnFiber(
 
     case HostPortal: {
       {
-        emptyPortalContainer(deletedFiber);
+        emptyPortalContainer();
         recursivelyTraverseDeletionEffects(
           finishedRoot,
           nearestMountedAncestor,
@@ -27063,7 +27124,7 @@ function createFiberRoot(
   return root;
 }
 
-var ReactVersion = "18.3.0-canary-112791ea";
+var ReactVersion = "18.3.0-canary-4b68b914";
 
 function createPortal$1(
   children,