@samitouri / QOS-React-1 / commits / c820097716

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.

Andrew Clark committed Feb 19, 2024 at 21:06 UTC c820097716c3d9765bf85bf58202a4975d99e450

3 files changed +90 -35

packages/react-reconciler/src/ReactFiberBeginWork.js

+3 -1

@@ -1007,6 +1007,8 @@ function updateProfiler(

1007 }

1008

1009 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.

1012 const ref = workInProgress.ref;

1013 if (

1014 (current === null && ref !== null) ||

@@ -3531,7 +3533,7 @@ function updateScopeComponent(

3533 ) {

3534 const nextProps = workInProgress.pendingProps;

3535 const nextChildren = nextProps.children;

3534 -

3536 + markRef(current, workInProgress);

3537 reconcileChildren(current, workInProgress, nextChildren, renderLanes);

3538 return workInProgress.child;

3539 }

packages/react-reconciler/src/ReactFiberCompleteWork.js

+4 -34

@@ -75,8 +75,6 @@ import {

75 } from './ReactWorkTags';

76 import {NoMode, ConcurrentMode, ProfileMode} from './ReactTypeOfMode';

77 import {

78 - Ref,

79 - RefStatic,

78 Placement,

79 Update,

80 Visibility,

@@ -186,10 +184,6 @@ function markUpdate(workInProgress: Fiber) {

184 workInProgress.flags |= Update;

185 }

186

189 -function markRef(workInProgress: Fiber) {

190 - workInProgress.flags |= Ref | RefStatic;

191 -}

192 -

187 /**

188 * In persistent mode, return whether this update needs to clone the subtree.

189 */

@@ -1083,9 +1077,6 @@ function completeWork(

1077 // @TODO refactor this block to create the instance here in complete

1078 // phase if we are not hydrating.

1079 markUpdate(workInProgress);

1086 - if (workInProgress.ref !== null) {

1087 - markRef(workInProgress);

1088 - }

1080 if (nextResource !== null) {

1081 // This is a Hoistable Resource

1082

@@ -1120,9 +1111,6 @@ function completeWork(

1111 // and require an update

1112 markUpdate(workInProgress);

1113 }

1123 - if (current.ref !== workInProgress.ref) {

1124 - markRef(workInProgress);

1125 - }

1114 if (nextResource !== null) {

1115 // This is a Hoistable Resource

1116 // This must come at the very end of the complete phase.

@@ -1194,10 +1182,6 @@ function completeWork(

1182 renderLanes,

1183 );

1184 }

1197 -

1198 - if (current.ref !== workInProgress.ref) {

1199 - markRef(workInProgress);

1200 - }

1185 } else {

1186 if (!newProps) {

1187 if (workInProgress.stateNode === null) {

@@ -1232,11 +1216,6 @@ function completeWork(

1216 workInProgress.stateNode = instance;

1217 markUpdate(workInProgress);

1218 }

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 - }

1219 }

1220 bubbleProperties(workInProgress);

1221 return null;

@@ -1254,10 +1233,6 @@ function completeWork(

1233 newProps,

1234 renderLanes,

1235 );

1257 -

1258 - if (current.ref !== workInProgress.ref) {

1259 - markRef(workInProgress);

1260 - }

1236 } else {

1237 if (!newProps) {

1238 if (workInProgress.stateNode === null) {

@@ -1310,11 +1285,6 @@ function completeWork(

1285 markUpdate(workInProgress);

1286 }

1287 }

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 - }

1288 }

1289 bubbleProperties(workInProgress);

1290

@@ -1739,16 +1709,16 @@ function completeWork(

1709 workInProgress.stateNode = scopeInstance;

1710 prepareScopeUpdate(scopeInstance, workInProgress);

1711 if (workInProgress.ref !== null) {

1742 - markRef(workInProgress);

1712 + // Scope components always do work in the commit phase if there's a

1713 + // ref attached.

1714 markUpdate(workInProgress);

1715 }

1716 } else {

1717 if (workInProgress.ref !== null) {

1718 + // Scope components always do work in the commit phase if there's a

1719 + // ref attached.

1720 markUpdate(workInProgress);

1721 }

1749 - if (current.ref !== workInProgress.ref) {

1750 - markRef(workInProgress);

1751 - }

1722 }

1723 bubbleProperties(workInProgress);

1724 return null;

packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js new

+83

@@ -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 +});