Skip to content

Commit c820097

Browse files
authored
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.
1 parent 2e84e16 commit c820097

File tree

3 files changed

+90
-35
lines changed

3 files changed

+90
-35
lines changed

packages/react-reconciler/src/ReactFiberBeginWork.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1007,6 +1007,8 @@ function updateProfiler(
10071007
}
10081008

10091009
function markRef(current: Fiber | null, workInProgress: Fiber) {
1010+
// TODO: This is also where we should check the type of the ref and error if
1011+
// an invalid one is passed, instead of during child reconcilation.
10101012
const ref = workInProgress.ref;
10111013
if (
10121014
(current === null && ref !== null) ||
@@ -3531,7 +3533,7 @@ function updateScopeComponent(
35313533
) {
35323534
const nextProps = workInProgress.pendingProps;
35333535
const nextChildren = nextProps.children;
3534-
3536+
markRef(current, workInProgress);
35353537
reconcileChildren(current, workInProgress, nextChildren, renderLanes);
35363538
return workInProgress.child;
35373539
}

packages/react-reconciler/src/ReactFiberCompleteWork.js

Lines changed: 4 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,6 @@ import {
7575
} from './ReactWorkTags';
7676
import {NoMode, ConcurrentMode, ProfileMode} from './ReactTypeOfMode';
7777
import {
78-
Ref,
79-
RefStatic,
8078
Placement,
8179
Update,
8280
Visibility,
@@ -186,10 +184,6 @@ function markUpdate(workInProgress: Fiber) {
186184
workInProgress.flags |= Update;
187185
}
188186

189-
function markRef(workInProgress: Fiber) {
190-
workInProgress.flags |= Ref | RefStatic;
191-
}
192-
193187
/**
194188
* In persistent mode, return whether this update needs to clone the subtree.
195189
*/
@@ -1083,9 +1077,6 @@ function completeWork(
10831077
// @TODO refactor this block to create the instance here in complete
10841078
// phase if we are not hydrating.
10851079
markUpdate(workInProgress);
1086-
if (workInProgress.ref !== null) {
1087-
markRef(workInProgress);
1088-
}
10891080
if (nextResource !== null) {
10901081
// This is a Hoistable Resource
10911082

@@ -1120,9 +1111,6 @@ function completeWork(
11201111
// and require an update
11211112
markUpdate(workInProgress);
11221113
}
1123-
if (current.ref !== workInProgress.ref) {
1124-
markRef(workInProgress);
1125-
}
11261114
if (nextResource !== null) {
11271115
// This is a Hoistable Resource
11281116
// This must come at the very end of the complete phase.
@@ -1194,10 +1182,6 @@ function completeWork(
11941182
renderLanes,
11951183
);
11961184
}
1197-
1198-
if (current.ref !== workInProgress.ref) {
1199-
markRef(workInProgress);
1200-
}
12011185
} else {
12021186
if (!newProps) {
12031187
if (workInProgress.stateNode === null) {
@@ -1232,11 +1216,6 @@ function completeWork(
12321216
workInProgress.stateNode = instance;
12331217
markUpdate(workInProgress);
12341218
}
1235-
1236-
if (workInProgress.ref !== null) {
1237-
// If there is a ref on a host node we need to schedule a callback
1238-
markRef(workInProgress);
1239-
}
12401219
}
12411220
bubbleProperties(workInProgress);
12421221
return null;
@@ -1254,10 +1233,6 @@ function completeWork(
12541233
newProps,
12551234
renderLanes,
12561235
);
1257-
1258-
if (current.ref !== workInProgress.ref) {
1259-
markRef(workInProgress);
1260-
}
12611236
} else {
12621237
if (!newProps) {
12631238
if (workInProgress.stateNode === null) {
@@ -1310,11 +1285,6 @@ function completeWork(
13101285
markUpdate(workInProgress);
13111286
}
13121287
}
1313-
1314-
if (workInProgress.ref !== null) {
1315-
// If there is a ref on a host node we need to schedule a callback
1316-
markRef(workInProgress);
1317-
}
13181288
}
13191289
bubbleProperties(workInProgress);
13201290

@@ -1739,16 +1709,16 @@ function completeWork(
17391709
workInProgress.stateNode = scopeInstance;
17401710
prepareScopeUpdate(scopeInstance, workInProgress);
17411711
if (workInProgress.ref !== null) {
1742-
markRef(workInProgress);
1712+
// Scope components always do work in the commit phase if there's a
1713+
// ref attached.
17431714
markUpdate(workInProgress);
17441715
}
17451716
} else {
17461717
if (workInProgress.ref !== null) {
1718+
// Scope components always do work in the commit phase if there's a
1719+
// ref attached.
17471720
markUpdate(workInProgress);
17481721
}
1749-
if (current.ref !== workInProgress.ref) {
1750-
markRef(workInProgress);
1751-
}
17521722
}
17531723
bubbleProperties(workInProgress);
17541724
return null;
Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
/**
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*
7+
* @emails react-core
8+
*/
9+
10+
'use strict';
11+
12+
let React;
13+
let Scheduler;
14+
let ReactNoop;
15+
let act;
16+
let assertLog;
17+
18+
describe('ReactFiberRefs', () => {
19+
beforeEach(() => {
20+
jest.resetModules();
21+
React = require('react');
22+
Scheduler = require('scheduler');
23+
ReactNoop = require('react-noop-renderer');
24+
act = require('internal-test-utils').act;
25+
assertLog = require('internal-test-utils').assertLog;
26+
});
27+
28+
test('ref is attached even if there are no other updates (class)', async () => {
29+
let component;
30+
class Component extends React.PureComponent {
31+
render() {
32+
Scheduler.log('Render');
33+
component = this;
34+
return 'Hi';
35+
}
36+
}
37+
38+
const ref1 = React.createRef();
39+
const ref2 = React.createRef();
40+
const root = ReactNoop.createRoot();
41+
42+
// Mount with ref1 attached
43+
await act(() => root.render(<Component ref={ref1} />));
44+
assertLog(['Render']);
45+
expect(root).toMatchRenderedOutput('Hi');
46+
expect(ref1.current).toBe(component);
47+
// ref2 has no value
48+
expect(ref2.current).toBe(null);
49+
50+
// Switch to ref2, but don't update anything else.
51+
await act(() => root.render(<Component ref={ref2} />));
52+
// The component did not re-render because no props changed.
53+
assertLog([]);
54+
expect(root).toMatchRenderedOutput('Hi');
55+
// But the refs still should have been swapped.
56+
expect(ref1.current).toBe(null);
57+
expect(ref2.current).toBe(component);
58+
});
59+
60+
test('ref is attached even if there are no other updates (host component)', async () => {
61+
// This is kind of ailly test because host components never bail out if they
62+
// receive a new element, and there's no way to update a ref without also
63+
// updating the props, but adding it here anyway for symmetry with the
64+
// class case above.
65+
const ref1 = React.createRef();
66+
const ref2 = React.createRef();
67+
const root = ReactNoop.createRoot();
68+
69+
// Mount with ref1 attached
70+
await act(() => root.render(<div ref={ref1}>Hi</div>));
71+
expect(root).toMatchRenderedOutput(<div>Hi</div>);
72+
expect(ref1.current).not.toBe(null);
73+
// ref2 has no value
74+
expect(ref2.current).toBe(null);
75+
76+
// Switch to ref2, but don't update anything else.
77+
await act(() => root.render(<div ref={ref2}>Hi</div>));
78+
expect(root).toMatchRenderedOutput(<div>Hi</div>);
79+
// But the refs still should have been swapped.
80+
expect(ref1.current).toBe(null);
81+
expect(ref2.current).not.toBe(null);
82+
});
83+
});

0 commit comments

Comments
 (0)