Move all markRef calls into begin phase (#28375)

Certain fiber types may have a ref attached to them. The main ones are HostComponent and ClassComponent. During the render phase, we check if a ref was passed to it, and if so, we schedule a Ref effect: `markRef`. Currently, we're not consistent about whether we call `markRef` in the begin phase or the complete phase. For some fiber types, I found that `markRef` was called in both phases, causing redundant work. After some investigation, I don't believe it's necessary to call `markRef` in both the begin phase and the complete phase, as long as you don't bail out before calling `markRef`. I though that maybe it had to do with the `attemptEarlyBailoutIfNoScheduledUpdates` branch, which is a fast path that skips the regular begin phase if no new props, state, or context were passed. But if the props haven't changed (referentially — the `memo` and `shouldComponentUpdate` checks happen later), then it follows that the ref couldn't have changed either. This is true even in the old `createElement` runtime where `ref` is stored on the element instead of as a prop, because there's no way to pass a new ref to an element without also passing new props. You might argue this is a leaky assumption, but since we're shifting ref to be just a regular prop anyway, I think it's the correct way to think about it going forward. I think the pattern of calling `markRef` in the complete phase may have been left over from an earlier iteration of the implementation before the bailout logic was structured like it is today. So, I removed all the `markRef` calls from the complete phase. In the case of ScopeComponent, which had no corresponding call in the begin phase, I added one. We already had a test that asserted that a ref is reattached even if the component bails out, but I added some new ones to be extra safe. The reason I'm changing this this is because I'm working on a different change to move the ref handling logic in `coerceRef` to happen in render phase of the component that accepts the ref, instead of during the parent's reconciliation. DiffTrain build for commit c820097.
facebook · Feb 20, 2024 · b125fcb · b125fcb
1 parent 469809a
commit b125fcb
Show file tree

Hide file tree

Showing 13 changed files with 240 additions and 286 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<<825f43f1ef38b03ee1f23d7689105b33>>
+ * @generated SignedSource<<bdf0df21b100941fad1618d2f0f4ea03>>
  */
 
 "use strict";
@@ -12911,7 +12911,7 @@ if (__DEV__) {
       var nextIsDetached =
         (workInProgress.stateNode._pendingVisibility & OffscreenDetached) !== 0;
       var prevState = current !== null ? current.memoizedState : null;
-      markRef$1(current, workInProgress);
+      markRef(current, workInProgress);
 
       if (nextProps.mode === "hidden" || enableLegacyHidden || nextIsDetached) {
         // Rendering a hidden tree.
@@ -13185,7 +13185,9 @@ if (__DEV__) {
       return workInProgress.child;
     }
 
-    function markRef$1(current, workInProgress) {
+    function markRef(current, workInProgress) {
+      // TODO: This is also where we should check the type of the ref and error if
+      // an invalid one is passed, instead of during child reconcilation.
       var ref = workInProgress.ref;
 
       if (
@@ -13403,7 +13405,7 @@ if (__DEV__) {
       renderLanes
     ) {
       // Refs should update even if shouldComponentUpdate returns false
-      markRef$1(current, workInProgress);
+      markRef(current, workInProgress);
       var didCaptureError = (workInProgress.flags & DidCapture) !== NoFlags$1;
 
       if (!shouldUpdate && !didCaptureError) {
@@ -13600,7 +13602,7 @@ if (__DEV__) {
         }
       }
 
-      markRef$1(current, workInProgress);
+      markRef(current, workInProgress);
       reconcileChildren(current, workInProgress, nextChildren, renderLanes);
       return workInProgress.child;
     }
@@ -16449,10 +16451,6 @@ if (__DEV__) {
       workInProgress.flags |= Update;
     }
 
-    function markRef(workInProgress) {
-      workInProgress.flags |= Ref | RefStatic;
-    }
-
     function appendAllChildren(
       parent,
       workInProgress,
@@ -16944,10 +16942,6 @@ if (__DEV__) {
 
           if (current !== null && workInProgress.stateNode != null) {
             updateHostComponent(current, workInProgress, _type2, newProps);
-
-            if (current.ref !== workInProgress.ref) {
-              markRef(workInProgress);
-            }
           } else {
             if (!newProps) {
               if (workInProgress.stateNode === null) {
@@ -16987,11 +16981,6 @@ if (__DEV__) {
               appendAllChildren(_instance3, workInProgress);
               workInProgress.stateNode = _instance3; // Certain renderers require commit-time effects for initial mount.
             }
-
-            if (workInProgress.ref !== null) {
-              // If there is a ref on a host node we need to schedule a callback
-              markRef(workInProgress);
-            }
           }
 
           bubbleProperties(workInProgress); // This must come at the very end of the complete phase, because it might
@@ -25697,7 +25686,7 @@ if (__DEV__) {
       return root;
     }
 
-    var ReactVersion = "18.3.0-canary-2e84e1629-20240219";
+    var ReactVersion = "18.3.0-canary-c82009771-20240219";
 
     // 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
@@ -7,7 +7,7 @@
  * @noflow
  * @nolint
  * @preventMunge
- * @generated SignedSource<<e0eb54caae0b3e65831e66b40e94cf62>>
+ * @generated SignedSource<<5572049a3941ae58bad5a378320ee208>>
  */
 
 "use strict";
@@ -3918,7 +3918,7 @@ function updateOffscreenComponent(current, workInProgress, renderLanes) {
     nextChildren = nextProps.children,
     nextIsDetached = 0 !== (workInProgress.stateNode._pendingVisibility & 2),
     prevState = null !== current ? current.memoizedState : null;
-  markRef$1(current, workInProgress);
+  markRef(current, workInProgress);
   if ("hidden" === nextProps.mode || nextIsDetached) {
     if (0 !== (workInProgress.flags & 128)) {
       renderLanes =
@@ -3989,7 +3989,7 @@ function deferHiddenOffscreenComponent(current, workInProgress, nextBaseLanes) {
   pushOffscreenSuspenseHandler(workInProgress);
   return null;
 }
-function markRef$1(current, workInProgress) {
+function markRef(current, workInProgress) {
   var ref = workInProgress.ref;
   if (
     (null === current && null !== ref) ||
@@ -4260,7 +4260,7 @@ function finishClassComponent(
   hasContext,
   renderLanes
 ) {
-  markRef$1(current, workInProgress);
+  markRef(current, workInProgress);
   var didCaptureError = 0 !== (workInProgress.flags & 128);
   if (!shouldUpdate && !didCaptureError)
     return (
@@ -5195,9 +5195,7 @@ function completeWork(current, workInProgress, renderLanes) {
       popHostContext(workInProgress);
       renderLanes = workInProgress.type;
       if (null !== current && null != workInProgress.stateNode)
-        current.memoizedProps !== newProps && (workInProgress.flags |= 4),
-          current.ref !== workInProgress.ref &&
-            (workInProgress.flags |= 2097664);
+        current.memoizedProps !== newProps && (workInProgress.flags |= 4);
       else {
         if (!newProps) {
           if (null === workInProgress.stateNode)
@@ -5240,7 +5238,6 @@ function completeWork(current, workInProgress, renderLanes) {
           renderLanes = renderLanes.sibling;
         }
         workInProgress.stateNode = current;
-        null !== workInProgress.ref && (workInProgress.flags |= 2097664);
       }
       bubbleProperties(workInProgress);
       workInProgress.flags &= -16777217;
@@ -8251,7 +8248,7 @@ beginWork = function (current, workInProgress, renderLanes) {
               HostTransitionContext,
               renderLanes
             )),
-        markRef$1(current, workInProgress),
+        markRef(current, workInProgress),
         reconcileChildren(current, workInProgress, Component, renderLanes),
         workInProgress.child
       );
@@ -9174,19 +9171,19 @@ function wrapFiber(fiber) {
     fiberToWrapper.set(fiber, wrapper));
   return wrapper;
 }
-var devToolsConfig$jscomp$inline_1018 = {
+var devToolsConfig$jscomp$inline_1014 = {
   findFiberByHostInstance: function () {
     throw Error("TestRenderer does not support findFiberByHostInstance()");
   },
   bundleType: 0,
-  version: "18.3.0-canary-2e84e1629-20240219",
+  version: "18.3.0-canary-c82009771-20240219",
   rendererPackageName: "react-test-renderer"
 };
-var internals$jscomp$inline_1199 = {
-  bundleType: devToolsConfig$jscomp$inline_1018.bundleType,
-  version: devToolsConfig$jscomp$inline_1018.version,
-  rendererPackageName: devToolsConfig$jscomp$inline_1018.rendererPackageName,
-  rendererConfig: devToolsConfig$jscomp$inline_1018.rendererConfig,
+var internals$jscomp$inline_1195 = {
+  bundleType: devToolsConfig$jscomp$inline_1014.bundleType,
+  version: devToolsConfig$jscomp$inline_1014.version,
+  rendererPackageName: devToolsConfig$jscomp$inline_1014.rendererPackageName,
+  rendererConfig: devToolsConfig$jscomp$inline_1014.rendererConfig,
   overrideHookState: null,
   overrideHookStateDeletePath: null,
   overrideHookStateRenamePath: null,
@@ -9203,26 +9200,26 @@ var internals$jscomp$inline_1199 = {
     return null === fiber ? null : fiber.stateNode;
   },
   findFiberByHostInstance:
-    devToolsConfig$jscomp$inline_1018.findFiberByHostInstance ||
+    devToolsConfig$jscomp$inline_1014.findFiberByHostInstance ||
     emptyFindFiberByHostInstance,
   findHostInstancesForRefresh: null,
   scheduleRefresh: null,
   scheduleRoot: null,
   setRefreshHandler: null,
   getCurrentFiber: null,
-  reconcilerVersion: "18.3.0-canary-2e84e1629-20240219"
+  reconcilerVersion: "18.3.0-canary-c82009771-20240219"
 };
 if ("undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__) {
-  var hook$jscomp$inline_1200 = __REACT_DEVTOOLS_GLOBAL_HOOK__;
+  var hook$jscomp$inline_1196 = __REACT_DEVTOOLS_GLOBAL_HOOK__;
   if (
-    !hook$jscomp$inline_1200.isDisabled &&
-    hook$jscomp$inline_1200.supportsFiber
+    !hook$jscomp$inline_1196.isDisabled &&
+    hook$jscomp$inline_1196.supportsFiber
   )
     try {
-      (rendererID = hook$jscomp$inline_1200.inject(
-        internals$jscomp$inline_1199
+      (rendererID = hook$jscomp$inline_1196.inject(
+        internals$jscomp$inline_1195
       )),
-        (injectedHook = hook$jscomp$inline_1200);
+        (injectedHook = hook$jscomp$inline_1196);
     } catch (err) {}
 }
 exports._Scheduler = Scheduler;

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
@@ -7,7 +7,7 @@
  * @noflow
  * @nolint
  * @preventMunge
- * @generated SignedSource<<30e0621a9f535ff1cd2b36a9447e4f81>>
+ * @generated SignedSource<<c8c04ec7f18e1d796a1ce5785c9676d3>>
  */
 
 "use strict";
@@ -4000,7 +4000,7 @@ function updateOffscreenComponent(current, workInProgress, renderLanes) {
     nextChildren = nextProps.children,
     nextIsDetached = 0 !== (workInProgress.stateNode._pendingVisibility & 2),
     prevState = null !== current ? current.memoizedState : null;
-  markRef$1(current, workInProgress);
+  markRef(current, workInProgress);
   if ("hidden" === nextProps.mode || nextIsDetached) {
     if (0 !== (workInProgress.flags & 128)) {
       renderLanes =
@@ -4071,7 +4071,7 @@ function deferHiddenOffscreenComponent(current, workInProgress, nextBaseLanes) {
   pushOffscreenSuspenseHandler(workInProgress);
   return null;
 }
-function markRef$1(current, workInProgress) {
+function markRef(current, workInProgress) {
   var ref = workInProgress.ref;
   if (
     (null === current && null !== ref) ||
@@ -4342,7 +4342,7 @@ function finishClassComponent(
   hasContext,
   renderLanes
 ) {
-  markRef$1(current, workInProgress);
+  markRef(current, workInProgress);
   var didCaptureError = 0 !== (workInProgress.flags & 128);
   if (!shouldUpdate && !didCaptureError)
     return (
@@ -5337,9 +5337,7 @@ function completeWork(current, workInProgress, renderLanes) {
       popHostContext(workInProgress);
       renderLanes = workInProgress.type;
       if (null !== current && null != workInProgress.stateNode)
-        current.memoizedProps !== newProps && (workInProgress.flags |= 4),
-          current.ref !== workInProgress.ref &&
-            (workInProgress.flags |= 2097664);
+        current.memoizedProps !== newProps && (workInProgress.flags |= 4);
       else {
         if (!newProps) {
           if (null === workInProgress.stateNode)
@@ -5382,7 +5380,6 @@ function completeWork(current, workInProgress, renderLanes) {
           renderLanes = renderLanes.sibling;
         }
         workInProgress.stateNode = current;
-        null !== workInProgress.ref && (workInProgress.flags |= 2097664);
       }
       bubbleProperties(workInProgress);
       workInProgress.flags &= -16777217;
@@ -8661,7 +8658,7 @@ beginWork = function (current, workInProgress, renderLanes) {
               HostTransitionContext,
               renderLanes
             )),
-        markRef$1(current, workInProgress),
+        markRef(current, workInProgress),
         reconcileChildren(current, workInProgress, Component, renderLanes),
         workInProgress.child
       );
@@ -9602,19 +9599,19 @@ function wrapFiber(fiber) {
     fiberToWrapper.set(fiber, wrapper));
   return wrapper;
 }
-var devToolsConfig$jscomp$inline_1060 = {
+var devToolsConfig$jscomp$inline_1056 = {
   findFiberByHostInstance: function () {
     throw Error("TestRenderer does not support findFiberByHostInstance()");
   },
   bundleType: 0,
-  version: "18.3.0-canary-2e84e1629-20240219",
+  version: "18.3.0-canary-c82009771-20240219",
   rendererPackageName: "react-test-renderer"
 };
-var internals$jscomp$inline_1240 = {
-  bundleType: devToolsConfig$jscomp$inline_1060.bundleType,
-  version: devToolsConfig$jscomp$inline_1060.version,
-  rendererPackageName: devToolsConfig$jscomp$inline_1060.rendererPackageName,
-  rendererConfig: devToolsConfig$jscomp$inline_1060.rendererConfig,
+var internals$jscomp$inline_1236 = {
+  bundleType: devToolsConfig$jscomp$inline_1056.bundleType,
+  version: devToolsConfig$jscomp$inline_1056.version,
+  rendererPackageName: devToolsConfig$jscomp$inline_1056.rendererPackageName,
+  rendererConfig: devToolsConfig$jscomp$inline_1056.rendererConfig,
   overrideHookState: null,
   overrideHookStateDeletePath: null,
   overrideHookStateRenamePath: null,
@@ -9631,26 +9628,26 @@ var internals$jscomp$inline_1240 = {
     return null === fiber ? null : fiber.stateNode;
   },
   findFiberByHostInstance:
-    devToolsConfig$jscomp$inline_1060.findFiberByHostInstance ||
+    devToolsConfig$jscomp$inline_1056.findFiberByHostInstance ||
     emptyFindFiberByHostInstance,
   findHostInstancesForRefresh: null,
   scheduleRefresh: null,
   scheduleRoot: null,
   setRefreshHandler: null,
   getCurrentFiber: null,
-  reconcilerVersion: "18.3.0-canary-2e84e1629-20240219"
+  reconcilerVersion: "18.3.0-canary-c82009771-20240219"
 };
 if ("undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__) {
-  var hook$jscomp$inline_1241 = __REACT_DEVTOOLS_GLOBAL_HOOK__;
+  var hook$jscomp$inline_1237 = __REACT_DEVTOOLS_GLOBAL_HOOK__;
   if (
-    !hook$jscomp$inline_1241.isDisabled &&
-    hook$jscomp$inline_1241.supportsFiber
+    !hook$jscomp$inline_1237.isDisabled &&
+    hook$jscomp$inline_1237.supportsFiber
   )
     try {
-      (rendererID = hook$jscomp$inline_1241.inject(
-        internals$jscomp$inline_1240
+      (rendererID = hook$jscomp$inline_1237.inject(
+        internals$jscomp$inline_1236
       )),
-        (injectedHook = hook$jscomp$inline_1241);
+        (injectedHook = hook$jscomp$inline_1237);
     } catch (err) {}
 }
 exports._Scheduler = Scheduler;

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
@@ -24,7 +24,7 @@ if (__DEV__) {
     ) {
       __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStart(new Error());
     }
-    var ReactVersion = "18.3.0-canary-2e84e1629-20240219";
+    var ReactVersion = "18.3.0-canary-c82009771-20240219";
 
     // 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
@@ -590,4 +590,4 @@ exports.useSyncExternalStore = function (
 exports.useTransition = function () {
   return ReactCurrentDispatcher.current.useTransition();
 };
-exports.version = "18.3.0-canary-2e84e1629-20240219";
+exports.version = "18.3.0-canary-c82009771-20240219";
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
@@ -586,7 +586,7 @@ exports.useSyncExternalStore = function (
 exports.useTransition = function () {
   return ReactCurrentDispatcher.current.useTransition();
 };
-exports.version = "18.3.0-canary-2e84e1629-20240219";
+exports.version = "18.3.0-canary-c82009771-20240219";
 "undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
   "function" ===
     typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

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 @@
-2e84e1629924e6cb278638305fa92040f6ef6eb5
+c820097716c3d9765bf85bf58202a4975d99e450
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		2e84e1629924e6cb278638305fa92040f6ef6eb5
		c820097716c3d9765bf85bf58202a4975d99e450