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 [c820097](c820097)
facebook · Feb 20, 2024 · 76bf9a9 · 76bf9a9
1 parent 3ff23f9
commit 76bf9a9
Show file tree

Hide file tree

Showing 19 changed files with 315 additions and 581 deletions.
diff --git a/compiled/facebook-www/REVISION b/compiled/facebook-www/REVISION
@@ -1 +1 @@
-2e84e1629924e6cb278638305fa92040f6ef6eb5
+c820097716c3d9765bf85bf58202a4975d99e450
diff --git a/compiled/facebook-www/React-prod.classic.js b/compiled/facebook-www/React-prod.classic.js
@@ -618,4 +618,4 @@ exports.useSyncExternalStore = function (
 exports.useTransition = function () {
   return ReactCurrentDispatcher.current.useTransition();
 };
-exports.version = "18.3.0-www-classic-3c4221fd";
+exports.version = "18.3.0-www-classic-31ea3e04";
diff --git a/compiled/facebook-www/React-profiling.classic.js b/compiled/facebook-www/React-profiling.classic.js
@@ -622,7 +622,7 @@ exports.useSyncExternalStore = function (
 exports.useTransition = function () {
   return ReactCurrentDispatcher.current.useTransition();
 };
-exports.version = "18.3.0-www-classic-7063c424";
+exports.version = "18.3.0-www-classic-3f55cb15";
 "undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
   "function" ===
     typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

diff --git a/compiled/facebook-www/ReactART-dev.classic.js b/compiled/facebook-www/ReactART-dev.classic.js
@@ -66,7 +66,7 @@ if (__DEV__) {
       return self;
     }
 
-    var ReactVersion = "18.3.0-www-classic-2901d413";
+    var ReactVersion = "18.3.0-www-classic-88364efd";
 
     var LegacyRoot = 0;
     var ConcurrentRoot = 1;
@@ -14886,7 +14886,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" ||
@@ -15244,7 +15244,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 (
@@ -15478,7 +15480,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) {
@@ -15708,7 +15710,7 @@ if (__DEV__) {
         }
       }
 
-      markRef$1(current, workInProgress);
+      markRef(current, workInProgress);
       reconcileChildren(current, workInProgress, nextChildren, renderLanes);
       return workInProgress.child;
     }
@@ -17451,6 +17453,7 @@ if (__DEV__) {
     function updateScopeComponent(current, workInProgress, renderLanes) {
       var nextProps = workInProgress.pendingProps;
       var nextChildren = nextProps.children;
+      markRef(current, workInProgress);
       reconcileChildren(current, workInProgress, nextChildren, renderLanes);
       return workInProgress.child;
     }
@@ -19283,10 +19286,6 @@ if (__DEV__) {
       workInProgress.flags |= Update;
     }
 
-    function markRef(workInProgress) {
-      workInProgress.flags |= Ref | RefStatic;
-    }
-
     function appendAllChildren(
       parent,
       workInProgress,
@@ -19803,10 +19802,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) {
@@ -19840,11 +19835,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
@@ -20273,17 +20263,16 @@ if (__DEV__) {
               prepareScopeUpdate();
 
               if (workInProgress.ref !== null) {
-                markRef(workInProgress);
+                // Scope components always do work in the commit phase if there's a
+                // ref attached.
                 markUpdate(workInProgress);
               }
             } else {
               if (workInProgress.ref !== null) {
+                // Scope components always do work in the commit phase if there's a
+                // ref attached.
                 markUpdate(workInProgress);
               }
-
-              if (current.ref !== workInProgress.ref) {
-                markRef(workInProgress);
-              }
             }
 
             bubbleProperties(workInProgress);

diff --git a/compiled/facebook-www/ReactART-dev.modern.js b/compiled/facebook-www/ReactART-dev.modern.js
@@ -66,7 +66,7 @@ if (__DEV__) {
       return self;
     }
 
-    var ReactVersion = "18.3.0-www-modern-827e818d";
+    var ReactVersion = "18.3.0-www-modern-eef60811";
 
     var LegacyRoot = 0;
     var ConcurrentRoot = 1;
@@ -14610,7 +14610,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" ||
@@ -14968,7 +14968,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 (
@@ -15192,7 +15194,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) {
@@ -15402,7 +15404,7 @@ if (__DEV__) {
         }
       }
 
-      markRef$1(current, workInProgress);
+      markRef(current, workInProgress);
       reconcileChildren(current, workInProgress, nextChildren, renderLanes);
       return workInProgress.child;
     }
@@ -17145,6 +17147,7 @@ if (__DEV__) {
     function updateScopeComponent(current, workInProgress, renderLanes) {
       var nextProps = workInProgress.pendingProps;
       var nextChildren = nextProps.children;
+      markRef(current, workInProgress);
       reconcileChildren(current, workInProgress, nextChildren, renderLanes);
       return workInProgress.child;
     }
@@ -18971,10 +18974,6 @@ if (__DEV__) {
       workInProgress.flags |= Update;
     }
 
-    function markRef(workInProgress) {
-      workInProgress.flags |= Ref | RefStatic;
-    }
-
     function appendAllChildren(
       parent,
       workInProgress,
@@ -19484,10 +19483,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) {
@@ -19521,11 +19516,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
@@ -19946,17 +19936,16 @@ if (__DEV__) {
               prepareScopeUpdate();
 
               if (workInProgress.ref !== null) {
-                markRef(workInProgress);
+                // Scope components always do work in the commit phase if there's a
+                // ref attached.
                 markUpdate(workInProgress);
               }
             } else {
               if (workInProgress.ref !== null) {
+                // Scope components always do work in the commit phase if there's a
+                // ref attached.
                 markUpdate(workInProgress);
               }
-
-              if (current.ref !== workInProgress.ref) {
-                markRef(workInProgress);
-              }
             }
 
             bubbleProperties(workInProgress);