Fix: Class components should "consume" ref prop (#28719)
When a ref is passed to a class component, the class instance is attached to the ref's current property automatically. This different from function components, where you have to do something extra to attach a ref to an instance, like passing the ref to `useImperativeHandle`. Existing class component code is written with the assumption that a ref will not be passed through as a prop. For example, class components that act as indirections often spread `this.props` onto a child component. To maintain this expectation, we should remove the ref from the props object ("consume" it) before passing it to lifecycle methods. Without this change, much existing code will break because the ref will attach to the inner component instead of the outer one. This is not an issue for function components because we used to warn if you passed a ref to a function component. Instead, you had to use `forwardRef`, which also implements this "consuming" behavior. There are a few places in the reconciler where we modify the fiber's internal props object before passing it to userspace. The trickiest one is class components, because the props object gets exposed in many different places, including as a property on the class instance. This was already accounted for when we added support for setting default props on a lazy wrapper (i.e. `React.lazy` that resolves to a class component). In all of these same places, we will also need to remove the ref prop when `enableRefAsProp` is on. Closes #28602 --------- Co-authored-by: Jan Kassens <jan@kassens.net>
Andrew Clark committed
Apr 2, 2024 at 23:15 UTC
dc545c8d6eaca87c8d5cabfab6e1c768ecafe426
8 files changed
+243
-51
packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
+3
-15
@@ -261,29 +261,17 @@ describe('ReactCompositeComponent', () => {
261
await act(() => {
262
root.render(<Component ref={refFn1} />);
263
});
264
- if (gate(flags => flags.enableRefAsProp)) {
265
- expect(instance1.props).toEqual({prop: 'testKey', ref: refFn1});
266
- } else {
267
- expect(instance1.props).toEqual({prop: 'testKey'});
268
- }
264
+ expect(instance1.props).toEqual({prop: 'testKey'});
265
266
await act(() => {
267
root.render(<Component ref={refFn2} prop={undefined} />);
268
});
273
- if (gate(flags => flags.enableRefAsProp)) {
274
- expect(instance2.props).toEqual({prop: 'testKey', ref: refFn2});
275
- } else {
276
- expect(instance2.props).toEqual({prop: 'testKey'});
277
- }
269
+ expect(instance2.props).toEqual({prop: 'testKey'});
270
271
await act(() => {
272
root.render(<Component ref={refFn3} prop={null} />);
273
});
282
- if (gate(flags => flags.enableRefAsProp)) {
283
- expect(instance3.props).toEqual({prop: null, ref: refFn3});
284
- } else {
285
- expect(instance3.props).toEqual({prop: null});
286
- }
274
+ expect(instance3.props).toEqual({prop: null});
275
});
276
277
it('should not mutate passed-in props object', async () => {
packages/react-reconciler/src/ReactFiberBeginWork.js
+20
-13
@@ -245,6 +245,7 @@ import {
245
mountClassInstance,
246
resumeMountClassInstance,
247
updateClassInstance,
248
+ resolveClassComponentProps,
249
} from './ReactFiberClassComponent';
250
import {resolveDefaultProps} from './ReactFiberLazyComponent';
251
import {
@@ -1762,9 +1763,9 @@ function mountLazyComponent(
1763
// Store the unwrapped component in the type.
1764
workInProgress.type = Component;
1765
1765
- const resolvedProps = resolveDefaultProps(Component, props);
1766
if (typeof Component === 'function') {
1767
if (isFunctionClassComponent(Component)) {
1768
+ const resolvedProps = resolveClassComponentProps(Component, props, false);
1769
workInProgress.tag = ClassComponent;
1770
if (__DEV__) {
1771
workInProgress.type = Component =
@@ -1778,6 +1779,7 @@ function mountLazyComponent(
1779
renderLanes,
1780
);
1781
} else {
1782
+ const resolvedProps = resolveDefaultProps(Component, props);
1783
workInProgress.tag = FunctionComponent;
1784
if (__DEV__) {
1785
validateFunctionComponentInDev(workInProgress, Component);
@@ -1795,6 +1797,7 @@ function mountLazyComponent(
1797
} else if (Component !== undefined && Component !== null) {
1798
const $$typeof = Component.$$typeof;
1799
if ($$typeof === REACT_FORWARD_REF_TYPE) {
1800
+ const resolvedProps = resolveDefaultProps(Component, props);
1801
workInProgress.tag = ForwardRef;
1802
if (__DEV__) {
1803
workInProgress.type = Component =
@@ -1808,6 +1811,7 @@ function mountLazyComponent(
1811
renderLanes,
1812
);
1813
} else if ($$typeof === REACT_MEMO_TYPE) {
1814
+ const resolvedProps = resolveDefaultProps(Component, props);
1815
workInProgress.tag = MemoComponent;
1816
return updateMemoComponent(
1817
null,
@@ -3938,10 +3942,11 @@ function beginWork(
3942
case ClassComponent: {
3943
const Component = workInProgress.type;
3944
const unresolvedProps = workInProgress.pendingProps;
3941
- const resolvedProps =
3942
- workInProgress.elementType === Component
3943
- ? unresolvedProps
3944
- : resolveDefaultProps(Component, unresolvedProps);
3945
+ const resolvedProps = resolveClassComponentProps(
3946
+ Component,
3947
+ unresolvedProps,
3948
+ workInProgress.elementType === Component,
3949
+ );
3950
return updateClassComponent(
3951
current,
3952
workInProgress,
@@ -4024,10 +4029,11 @@ function beginWork(
4029
}
4030
const Component = workInProgress.type;
4031
const unresolvedProps = workInProgress.pendingProps;
4027
- const resolvedProps =
4028
- workInProgress.elementType === Component
4029
- ? unresolvedProps
4030
- : resolveDefaultProps(Component, unresolvedProps);
4032
+ const resolvedProps = resolveClassComponentProps(
4033
+ Component,
4034
+ unresolvedProps,
4035
+ workInProgress.elementType === Component,
4036
+ );
4037
return mountIncompleteClassComponent(
4038
current,
4039
workInProgress,
@@ -4042,10 +4048,11 @@ function beginWork(
4048
}
4049
const Component = workInProgress.type;
4050
const unresolvedProps = workInProgress.pendingProps;
4045
- const resolvedProps =
4046
- workInProgress.elementType === Component
4047
- ? unresolvedProps
4048
- : resolveDefaultProps(Component, unresolvedProps);
4051
+ const resolvedProps = resolveClassComponentProps(
4052
+ Component,
4053
+ unresolvedProps,
4054
+ workInProgress.elementType === Component,
4055
+ );
4056
return mountIncompleteFunctionComponent(
4057
current,
4058
workInProgress,
packages/react-reconciler/src/ReactFiberClassComponent.js
+57
-8
@@ -23,6 +23,7 @@ import {
23
enableDebugTracing,
24
enableSchedulingProfiler,
25
enableLazyContextPropagation,
26
+ enableRefAsProp,
27
} from 'shared/ReactFeatureFlags';
28
import ReactStrictModeWarnings from './ReactStrictModeWarnings';
29
import {isMounted} from './ReactFiberTreeReflection';
@@ -34,7 +35,6 @@ import assign from 'shared/assign';
35
import isArray from 'shared/isArray';
36
import {REACT_CONTEXT_TYPE, REACT_CONSUMER_TYPE} from 'shared/ReactSymbols';
37
37
-import {resolveDefaultProps} from './ReactFiberLazyComponent';
38
import {
39
DebugTracingMode,
40
NoMode,
@@ -904,7 +904,12 @@ function resumeMountClassInstance(
904
): boolean {
905
const instance = workInProgress.stateNode;
906
907
- const oldProps = workInProgress.memoizedProps;
907
+ const unresolvedOldProps = workInProgress.memoizedProps;
908
+ const oldProps = resolveClassComponentProps(
909
+ ctor,
910
+ unresolvedOldProps,
911
+ workInProgress.type === workInProgress.elementType,
912
+ );
913
instance.props = oldProps;
914
915
const oldContext = instance.context;
@@ -926,6 +931,13 @@ function resumeMountClassInstance(
931
typeof getDerivedStateFromProps === 'function' ||
932
typeof instance.getSnapshotBeforeUpdate === 'function';
933
934
+ // When comparing whether props changed, we should compare using the
935
+ // unresolved props object that is stored on the fiber, rather than the
936
+ // one that gets assigned to the instance, because that object may have been
937
+ // cloned to resolve default props and/or remove `ref`.
938
+ const unresolvedNewProps = workInProgress.pendingProps;
939
+ const didReceiveNewProps = unresolvedNewProps !== unresolvedOldProps;
940
+
941
// Note: During these life-cycles, instance.props/instance.state are what
942
// ever the previously attempted to render - not the "current". However,
943
// during componentDidUpdate we pass the "current" props.
@@ -937,7 +949,7 @@ function resumeMountClassInstance(
949
(typeof instance.UNSAFE_componentWillReceiveProps === 'function' ||
950
typeof instance.componentWillReceiveProps === 'function')
951
) {
940
- if (oldProps !== newProps || oldContext !== nextContext) {
952
+ if (didReceiveNewProps || oldContext !== nextContext) {
953
callComponentWillReceiveProps(
954
workInProgress,
955
instance,
@@ -955,7 +967,7 @@ function resumeMountClassInstance(
967
suspendIfUpdateReadFromEntangledAsyncAction();
968
newState = workInProgress.memoizedState;
969
if (
958
- oldProps === newProps &&
970
+ !didReceiveNewProps &&
971
oldState === newState &&
972
!hasContextChanged() &&
973
!checkHasForceUpdateAfterProcessing()
@@ -1052,10 +1064,11 @@ function updateClassInstance(
1064
cloneUpdateQueue(current, workInProgress);
1065
1066
const unresolvedOldProps = workInProgress.memoizedProps;
1055
- const oldProps =
1056
- workInProgress.type === workInProgress.elementType
1057
- ? unresolvedOldProps
1058
- : resolveDefaultProps(workInProgress.type, unresolvedOldProps);
1067
+ const oldProps = resolveClassComponentProps(
1068
+ ctor,
1069
+ unresolvedOldProps,
1070
+ workInProgress.type === workInProgress.elementType,
1071
+ );
1072
instance.props = oldProps;
1073
const unresolvedNewProps = workInProgress.pendingProps;
1074
@@ -1225,6 +1238,42 @@ function updateClassInstance(
1238
return shouldUpdate;
1239
}
1240
1241
+export function resolveClassComponentProps(
1242
+ Component: any,
1243
+ baseProps: Object,
1244
+ // Only resolve default props if this is a lazy component. Otherwise, they
1245
+ // would have already been resolved by the JSX runtime.
1246
+ // TODO: We're going to remove default prop resolution from the JSX runtime
1247
+ // and keep it only for class components. As part of that change, we should
1248
+ // remove this extra check.
1249
+ alreadyResolvedDefaultProps: boolean,
1250
+): Object {
1251
+ let newProps = baseProps;
1252
+
1253
+ // Resolve default props. Taken from old JSX runtime, where this used to live.
1254
+ const defaultProps = Component.defaultProps;
1255
+ if (defaultProps && !alreadyResolvedDefaultProps) {
1256
+ newProps = assign({}, newProps, baseProps);
1257
+ for (const propName in defaultProps) {
1258
+ if (newProps[propName] === undefined) {
1259
+ newProps[propName] = defaultProps[propName];
1260
+ }
1261
+ }
1262
+ }
1263
+
1264
+ if (enableRefAsProp) {
1265
+ // Remove ref from the props object, if it exists.
1266
+ if ('ref' in newProps) {
1267
+ if (newProps === baseProps) {
1268
+ newProps = assign({}, newProps);
1269
+ }
1270
+ delete newProps.ref;
1271
+ }
1272
+ }
1273
+
1274
+ return newProps;
1275
+}
1276
+
1277
export {
1278
constructClassInstance,
1279
mountClassInstance,
packages/react-reconciler/src/ReactFiberCommitWork.js
+24
-13
@@ -104,7 +104,7 @@ import {
104
setCurrentFiber as setCurrentDebugFiberInDEV,
105
getCurrentFiber as getCurrentDebugFiberInDEV,
106
} from './ReactCurrentFiber';
107
-import {resolveDefaultProps} from './ReactFiberLazyComponent';
107
+import {resolveClassComponentProps} from './ReactFiberClassComponent';
108
import {
109
isCurrentUpdateNested,
110
getCommitTime,
@@ -244,7 +244,11 @@ function shouldProfile(current: Fiber): boolean {
244
}
245
246
function callComponentWillUnmountWithTimer(current: Fiber, instance: any) {
247
- instance.props = current.memoizedProps;
247
+ instance.props = resolveClassComponentProps(
248
+ current.type,
249
+ current.memoizedProps,
250
+ current.elementType === current.type,
251
+ );
252
instance.state = current.memoizedState;
253
if (shouldProfile(current)) {
254
try {
@@ -471,7 +475,8 @@ function commitBeforeMutationEffectsOnFiber(finishedWork: Fiber) {
475
// TODO: revisit this when we implement resuming.
476
if (__DEV__) {
477
if (
474
- finishedWork.type === finishedWork.elementType &&
478
+ !finishedWork.type.defaultProps &&
479
+ !('ref' in finishedWork.memoizedProps) &&
480
!didWarnAboutReassigningProps
481
) {
482
if (instance.props !== finishedWork.memoizedProps) {
@@ -497,9 +502,11 @@ function commitBeforeMutationEffectsOnFiber(finishedWork: Fiber) {
502
}
503
}
504
const snapshot = instance.getSnapshotBeforeUpdate(
500
- finishedWork.elementType === finishedWork.type
501
- ? prevProps
502
- : resolveDefaultProps(finishedWork.type, prevProps),
505
+ resolveClassComponentProps(
506
+ finishedWork.type,
507
+ prevProps,
508
+ finishedWork.elementType === finishedWork.type,
509
+ ),
510
prevState,
511
);
512
if (__DEV__) {
@@ -807,7 +814,8 @@ function commitClassLayoutLifecycles(
814
// TODO: revisit this when we implement resuming.
815
if (__DEV__) {
816
if (
810
- finishedWork.type === finishedWork.elementType &&
817
+ !finishedWork.type.defaultProps &&
818
+ !('ref' in finishedWork.memoizedProps) &&
819
!didWarnAboutReassigningProps
820
) {
821
if (instance.props !== finishedWork.memoizedProps) {
@@ -848,17 +856,19 @@ function commitClassLayoutLifecycles(
856
}
857
}
858
} else {
851
- const prevProps =
852
- finishedWork.elementType === finishedWork.type
853
- ? current.memoizedProps
854
- : resolveDefaultProps(finishedWork.type, current.memoizedProps);
859
+ const prevProps = resolveClassComponentProps(
860
+ finishedWork.type,
861
+ current.memoizedProps,
862
+ finishedWork.elementType === finishedWork.type,
863
+ );
864
const prevState = current.memoizedState;
865
// We could update instance props and state here,
866
// but instead we rely on them being set during last render.
867
// TODO: revisit this when we implement resuming.
868
if (__DEV__) {
869
if (
861
- finishedWork.type === finishedWork.elementType &&
870
+ !finishedWork.type.defaultProps &&
871
+ !('ref' in finishedWork.memoizedProps) &&
872
!didWarnAboutReassigningProps
873
) {
874
if (instance.props !== finishedWork.memoizedProps) {
@@ -918,7 +928,8 @@ function commitClassCallbacks(finishedWork: Fiber) {
928
const instance = finishedWork.stateNode;
929
if (__DEV__) {
930
if (
921
- finishedWork.type === finishedWork.elementType &&
931
+ !finishedWork.type.defaultProps &&
932
+ !('ref' in finishedWork.memoizedProps) &&
933
!didWarnAboutReassigningProps
934
) {
935
if (instance.props !== finishedWork.memoizedProps) {
packages/react-reconciler/src/ReactFiberLazyComponent.js
+4
@@ -10,6 +10,10 @@
10
import assign from 'shared/assign';
11
12
export function resolveDefaultProps(Component: any, baseProps: Object): Object {
13
+ // TODO: Remove support for default props for everything except class
14
+ // components, including setting default props on a lazy wrapper around a
15
+ // class type.
16
+
17
if (Component && Component.defaultProps) {
18
// Resolve default props. Taken from ReactElement
19
const props = assign({}, baseProps);
packages/react-reconciler/src/__tests__/ReactClassComponentPropResolution-test.js
new
+133
@@ -0,0 +1,133 @@
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 ReactNoop;
14
+let Scheduler;
15
+let act;
16
+let assertLog;
17
+
18
+describe('ReactClassComponentPropResolution', () => {
19
+ beforeEach(() => {
20
+ jest.resetModules();
21
+
22
+ React = require('react');
23
+ ReactNoop = require('react-noop-renderer');
24
+ Scheduler = require('scheduler');
25
+ act = require('internal-test-utils').act;
26
+ assertLog = require('internal-test-utils').assertLog;
27
+ });
28
+
29
+ function Text({text}) {
30
+ Scheduler.log(text);
31
+ return text;
32
+ }
33
+
34
+ test('resolves ref and default props before calling lifecycle methods', async () => {
35
+ const root = ReactNoop.createRoot();
36
+
37
+ function getPropKeys(props) {
38
+ return Object.keys(props).join(', ');
39
+ }
40
+
41
+ class Component extends React.Component {
42
+ constructor(props) {
43
+ super(props);
44
+ Scheduler.log('constructor: ' + getPropKeys(props));
45
+ }
46
+ shouldComponentUpdate(props) {
47
+ Scheduler.log(
48
+ 'shouldComponentUpdate (prev props): ' + getPropKeys(this.props),
49
+ );
50
+ Scheduler.log(
51
+ 'shouldComponentUpdate (next props): ' + getPropKeys(props),
52
+ );
53
+ return true;
54
+ }
55
+ componentDidUpdate(props) {
56
+ Scheduler.log('componentDidUpdate (prev props): ' + getPropKeys(props));
57
+ Scheduler.log(
58
+ 'componentDidUpdate (next props): ' + getPropKeys(this.props),
59
+ );
60
+ return true;
61
+ }
62
+ componentDidMount() {
63
+ Scheduler.log('componentDidMount: ' + getPropKeys(this.props));
64
+ return true;
65
+ }
66
+ UNSAFE_componentWillMount() {
67
+ Scheduler.log('componentWillMount: ' + getPropKeys(this.props));
68
+ }
69
+ UNSAFE_componentWillReceiveProps(nextProps) {
70
+ Scheduler.log(
71
+ 'componentWillReceiveProps (prev props): ' + getPropKeys(this.props),
72
+ );
73
+ Scheduler.log(
74
+ 'componentWillReceiveProps (next props): ' + getPropKeys(nextProps),
75
+ );
76
+ }
77
+ UNSAFE_componentWillUpdate(nextProps) {
78
+ Scheduler.log(
79
+ 'componentWillUpdate (prev props): ' + getPropKeys(this.props),
80
+ );
81
+ Scheduler.log(
82
+ 'componentWillUpdate (next props): ' + getPropKeys(nextProps),
83
+ );
84
+ }
85
+ componentWillUnmount() {
86
+ Scheduler.log('componentWillUnmount: ' + getPropKeys(this.props));
87
+ }
88
+ render() {
89
+ return <Text text={'render: ' + getPropKeys(this.props)} />;
90
+ }
91
+ }
92
+
93
+ Component.defaultProps = {
94
+ default: 'yo',
95
+ };
96
+
97
+ // `ref` should never appear as a prop. `default` always should.
98
+
99
+ // Mount
100
+ const ref = React.createRef();
101
+ await act(async () => {
102
+ root.render(<Component text="Yay" ref={ref} />);
103
+ });
104
+ assertLog([
105
+ 'constructor: text, default',
106
+ 'componentWillMount: text, default',
107
+ 'render: text, default',
108
+ 'componentDidMount: text, default',
109
+ ]);
110
+
111
+ // Update
112
+ await act(async () => {
113
+ root.render(<Component text="Yay (again)" ref={ref} />);
114
+ });
115
+ assertLog([
116
+ 'componentWillReceiveProps (prev props): text, default',
117
+ 'componentWillReceiveProps (next props): text, default',
118
+ 'shouldComponentUpdate (prev props): text, default',
119
+ 'shouldComponentUpdate (next props): text, default',
120
+ 'componentWillUpdate (prev props): text, default',
121
+ 'componentWillUpdate (next props): text, default',
122
+ 'render: text, default',
123
+ 'componentDidUpdate (prev props): text, default',
124
+ 'componentDidUpdate (next props): text, default',
125
+ ]);
126
+
127
+ // Unmount
128
+ await act(async () => {
129
+ root.render(null);
130
+ });
131
+ assertLog(['componentWillUnmount: text, default']);
132
+ });
133
+});
packages/react/src/__tests__/ReactCreateElement-test.js
+1
-1
@@ -90,7 +90,7 @@ describe('ReactCreateElement', () => {
90
);
91
});
92
93
- // @gate !enableRefAsProp
93
+ // @gate !enableRefAsProp || !__DEV__
94
it('should warn when `ref` is being accessed', async () => {
95
class Child extends React.Component {
96
render() {
packages/react/src/__tests__/ReactJSXRuntime-test.js
+1
-1
@@ -244,7 +244,7 @@ describe('ReactJSXRuntime', () => {
244
);
245
});
246
247
- // @gate !enableRefAsProp
247
+ // @gate !enableRefAsProp || !__DEV__
248
it('should warn when `ref` is being accessed', async () => {
249
const container = document.createElement('div');
250
class Child extends React.Component {