Skip to content

Commit 76bf9a9

Browse files
acdliteacdlite
committed
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)
1 parent 3ff23f9 commit 76bf9a9

19 files changed

+315
-581
lines changed

‎compiled/facebook-www/REVISION

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
2e84e1629924e6cb278638305fa92040f6ef6eb5
1+
c820097716c3d9765bf85bf58202a4975d99e450

‎compiled/facebook-www/React-prod.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -618,4 +618,4 @@ exports.useSyncExternalStore = function (
618618
exports.useTransition = function () {
619619
return ReactCurrentDispatcher.current.useTransition();
620620
};
621-
exports.version = "18.3.0-www-classic-3c4221fd";
621+
exports.version = "18.3.0-www-classic-31ea3e04";

‎compiled/facebook-www/React-profiling.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -622,7 +622,7 @@ exports.useSyncExternalStore = function (
622622
exports.useTransition = function () {
623623
return ReactCurrentDispatcher.current.useTransition();
624624
};
625-
exports.version = "18.3.0-www-classic-7063c424";
625+
exports.version = "18.3.0-www-classic-3f55cb15";
626626
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
627627
"function" ===
628628
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

‎compiled/facebook-www/ReactART-dev.classic.js

Lines changed: 12 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ if (__DEV__) {
6666
return self;
6767
}
6868

69-
var ReactVersion = "18.3.0-www-classic-2901d413";
69+
var ReactVersion = "18.3.0-www-classic-88364efd";
7070

7171
var LegacyRoot = 0;
7272
var ConcurrentRoot = 1;
@@ -14886,7 +14886,7 @@ if (__DEV__) {
1488614886
var nextIsDetached =
1488714887
(workInProgress.stateNode._pendingVisibility & OffscreenDetached) !== 0;
1488814888
var prevState = current !== null ? current.memoizedState : null;
14889-
markRef$1(current, workInProgress);
14889+
markRef(current, workInProgress);
1489014890

1489114891
if (
1489214892
nextProps.mode === "hidden" ||
@@ -15244,7 +15244,9 @@ if (__DEV__) {
1524415244
return workInProgress.child;
1524515245
}
1524615246

15247-
function markRef$1(current, workInProgress) {
15247+
function markRef(current, workInProgress) {
15248+
// TODO: This is also where we should check the type of the ref and error if
15249+
// an invalid one is passed, instead of during child reconcilation.
1524815250
var ref = workInProgress.ref;
1524915251

1525015252
if (
@@ -15478,7 +15480,7 @@ if (__DEV__) {
1547815480
renderLanes
1547915481
) {
1548015482
// Refs should update even if shouldComponentUpdate returns false
15481-
markRef$1(current, workInProgress);
15483+
markRef(current, workInProgress);
1548215484
var didCaptureError = (workInProgress.flags & DidCapture) !== NoFlags$1;
1548315485

1548415486
if (!shouldUpdate && !didCaptureError) {
@@ -15708,7 +15710,7 @@ if (__DEV__) {
1570815710
}
1570915711
}
1571015712

15711-
markRef$1(current, workInProgress);
15713+
markRef(current, workInProgress);
1571215714
reconcileChildren(current, workInProgress, nextChildren, renderLanes);
1571315715
return workInProgress.child;
1571415716
}
@@ -17451,6 +17453,7 @@ if (__DEV__) {
1745117453
function updateScopeComponent(current, workInProgress, renderLanes) {
1745217454
var nextProps = workInProgress.pendingProps;
1745317455
var nextChildren = nextProps.children;
17456+
markRef(current, workInProgress);
1745417457
reconcileChildren(current, workInProgress, nextChildren, renderLanes);
1745517458
return workInProgress.child;
1745617459
}
@@ -19283,10 +19286,6 @@ if (__DEV__) {
1928319286
workInProgress.flags |= Update;
1928419287
}
1928519288

19286-
function markRef(workInProgress) {
19287-
workInProgress.flags |= Ref | RefStatic;
19288-
}
19289-
1929019289
function appendAllChildren(
1929119290
parent,
1929219291
workInProgress,
@@ -19803,10 +19802,6 @@ if (__DEV__) {
1980319802

1980419803
if (current !== null && workInProgress.stateNode != null) {
1980519804
updateHostComponent(current, workInProgress, _type2, newProps);
19806-
19807-
if (current.ref !== workInProgress.ref) {
19808-
markRef(workInProgress);
19809-
}
1981019805
} else {
1981119806
if (!newProps) {
1981219807
if (workInProgress.stateNode === null) {
@@ -19840,11 +19835,6 @@ if (__DEV__) {
1984019835
appendAllChildren(_instance3, workInProgress);
1984119836
workInProgress.stateNode = _instance3; // Certain renderers require commit-time effects for initial mount.
1984219837
}
19843-
19844-
if (workInProgress.ref !== null) {
19845-
// If there is a ref on a host node we need to schedule a callback
19846-
markRef(workInProgress);
19847-
}
1984819838
}
1984919839

1985019840
bubbleProperties(workInProgress); // This must come at the very end of the complete phase, because it might
@@ -20273,17 +20263,16 @@ if (__DEV__) {
2027320263
prepareScopeUpdate();
2027420264

2027520265
if (workInProgress.ref !== null) {
20276-
markRef(workInProgress);
20266+
// Scope components always do work in the commit phase if there's a
20267+
// ref attached.
2027720268
markUpdate(workInProgress);
2027820269
}
2027920270
} else {
2028020271
if (workInProgress.ref !== null) {
20272+
// Scope components always do work in the commit phase if there's a
20273+
// ref attached.
2028120274
markUpdate(workInProgress);
2028220275
}
20283-
20284-
if (current.ref !== workInProgress.ref) {
20285-
markRef(workInProgress);
20286-
}
2028720276
}
2028820277

2028920278
bubbleProperties(workInProgress);

‎compiled/facebook-www/ReactART-dev.modern.js

Lines changed: 12 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ if (__DEV__) {
6666
return self;
6767
}
6868

69-
var ReactVersion = "18.3.0-www-modern-827e818d";
69+
var ReactVersion = "18.3.0-www-modern-eef60811";
7070

7171
var LegacyRoot = 0;
7272
var ConcurrentRoot = 1;
@@ -14610,7 +14610,7 @@ if (__DEV__) {
1461014610
var nextIsDetached =
1461114611
(workInProgress.stateNode._pendingVisibility & OffscreenDetached) !== 0;
1461214612
var prevState = current !== null ? current.memoizedState : null;
14613-
markRef$1(current, workInProgress);
14613+
markRef(current, workInProgress);
1461414614

1461514615
if (
1461614616
nextProps.mode === "hidden" ||
@@ -14968,7 +14968,9 @@ if (__DEV__) {
1496814968
return workInProgress.child;
1496914969
}
1497014970

14971-
function markRef$1(current, workInProgress) {
14971+
function markRef(current, workInProgress) {
14972+
// TODO: This is also where we should check the type of the ref and error if
14973+
// an invalid one is passed, instead of during child reconcilation.
1497214974
var ref = workInProgress.ref;
1497314975

1497414976
if (
@@ -15192,7 +15194,7 @@ if (__DEV__) {
1519215194
renderLanes
1519315195
) {
1519415196
// Refs should update even if shouldComponentUpdate returns false
15195-
markRef$1(current, workInProgress);
15197+
markRef(current, workInProgress);
1519615198
var didCaptureError = (workInProgress.flags & DidCapture) !== NoFlags$1;
1519715199

1519815200
if (!shouldUpdate && !didCaptureError) {
@@ -15402,7 +15404,7 @@ if (__DEV__) {
1540215404
}
1540315405
}
1540415406

15405-
markRef$1(current, workInProgress);
15407+
markRef(current, workInProgress);
1540615408
reconcileChildren(current, workInProgress, nextChildren, renderLanes);
1540715409
return workInProgress.child;
1540815410
}
@@ -17145,6 +17147,7 @@ if (__DEV__) {
1714517147
function updateScopeComponent(current, workInProgress, renderLanes) {
1714617148
var nextProps = workInProgress.pendingProps;
1714717149
var nextChildren = nextProps.children;
17150+
markRef(current, workInProgress);
1714817151
reconcileChildren(current, workInProgress, nextChildren, renderLanes);
1714917152
return workInProgress.child;
1715017153
}
@@ -18971,10 +18974,6 @@ if (__DEV__) {
1897118974
workInProgress.flags |= Update;
1897218975
}
1897318976

18974-
function markRef(workInProgress) {
18975-
workInProgress.flags |= Ref | RefStatic;
18976-
}
18977-
1897818977
function appendAllChildren(
1897918978
parent,
1898018979
workInProgress,
@@ -19484,10 +19483,6 @@ if (__DEV__) {
1948419483

1948519484
if (current !== null && workInProgress.stateNode != null) {
1948619485
updateHostComponent(current, workInProgress, _type2, newProps);
19487-
19488-
if (current.ref !== workInProgress.ref) {
19489-
markRef(workInProgress);
19490-
}
1949119486
} else {
1949219487
if (!newProps) {
1949319488
if (workInProgress.stateNode === null) {
@@ -19521,11 +19516,6 @@ if (__DEV__) {
1952119516
appendAllChildren(_instance3, workInProgress);
1952219517
workInProgress.stateNode = _instance3; // Certain renderers require commit-time effects for initial mount.
1952319518
}
19524-
19525-
if (workInProgress.ref !== null) {
19526-
// If there is a ref on a host node we need to schedule a callback
19527-
markRef(workInProgress);
19528-
}
1952919519
}
1953019520

1953119521
bubbleProperties(workInProgress); // This must come at the very end of the complete phase, because it might
@@ -19946,17 +19936,16 @@ if (__DEV__) {
1994619936
prepareScopeUpdate();
1994719937

1994819938
if (workInProgress.ref !== null) {
19949-
markRef(workInProgress);
19939+
// Scope components always do work in the commit phase if there's a
19940+
// ref attached.
1995019941
markUpdate(workInProgress);
1995119942
}
1995219943
} else {
1995319944
if (workInProgress.ref !== null) {
19945+
// Scope components always do work in the commit phase if there's a
19946+
// ref attached.
1995419947
markUpdate(workInProgress);
1995519948
}
19956-
19957-
if (current.ref !== workInProgress.ref) {
19958-
markRef(workInProgress);
19959-
}
1996019949
}
1996119950

1996219951
bubbleProperties(workInProgress);

0 commit comments

Comments
 (0)