From ec538219fac2cdbd401e1d5aa52b9b70beb459cd Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 2 Apr 2024 13:21:58 -0400 Subject: [PATCH] Fix: Class components should "consume" ref prop 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. Co-authored-by: Jan Kassens --- .../__tests__/ReactCompositeComponent-test.js | 18 +--- .../src/ReactFiberClassComponent.js | 11 ++- .../src/ReactFiberCommitWork.js | 19 +++- .../ReactClassComponentPropResolution-test.js | 92 +++++++++++++++++++ .../src/__tests__/ReactCreateElement-test.js | 2 +- 5 files changed, 122 insertions(+), 20 deletions(-) create mode 100644 packages/react-reconciler/src/__tests__/ReactClassComponentPropResolution-test.js diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js index 5959cdefccfe3..7fdeb5c2eb4c7 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js @@ -268,29 +268,17 @@ describe('ReactCompositeComponent', () => { await act(() => { root.render(); }); - if (gate(flags => flags.enableRefAsProp)) { - expect(instance1.props).toEqual({prop: 'testKey', ref: refFn1}); - } else { - expect(instance1.props).toEqual({prop: 'testKey'}); - } + expect(instance1.props).toEqual({prop: 'testKey'}); await act(() => { root.render(); }); - if (gate(flags => flags.enableRefAsProp)) { - expect(instance2.props).toEqual({prop: 'testKey', ref: refFn2}); - } else { - expect(instance2.props).toEqual({prop: 'testKey'}); - } + expect(instance2.props).toEqual({prop: 'testKey'}); await act(() => { root.render(); }); - if (gate(flags => flags.enableRefAsProp)) { - expect(instance3.props).toEqual({prop: null, ref: refFn3}); - } else { - expect(instance3.props).toEqual({prop: null}); - } + expect(instance3.props).toEqual({prop: null}); }); it('should not mutate passed-in props object', async () => { diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 96a34ac6f03ae..bd0caf1e42e96 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -23,6 +23,7 @@ import { enableDebugTracing, enableSchedulingProfiler, enableLazyContextPropagation, + enableRefAsProp, } from 'shared/ReactFeatureFlags'; import ReactStrictModeWarnings from './ReactStrictModeWarnings'; import {isMounted} from './ReactFiberTreeReflection'; @@ -1252,7 +1253,15 @@ export function resolveClassComponentProps( } } - // TODO: Remove ref from props object + if (enableRefAsProp) { + // Remove ref from the props object, if it exists. + if ('ref' in newProps) { + if (newProps === baseProps) { + newProps = assign({}, newProps); + } + delete newProps.ref; + } + } return newProps; } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 5ca704193b99c..458e900601971 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -471,6 +471,7 @@ function commitBeforeMutationEffectsOnFiber(finishedWork: Fiber) { if (__DEV__) { if ( !finishedWork.type.defaultProps && + !('ref' in finishedWork.memoizedProps) && !didWarnAboutReassigningProps ) { if (instance.props !== finishedWork.memoizedProps) { @@ -807,7 +808,11 @@ function commitClassLayoutLifecycles( // but instead we rely on them being set during last render. // TODO: revisit this when we implement resuming. if (__DEV__) { - if (!finishedWork.type.defaultProps && !didWarnAboutReassigningProps) { + if ( + !finishedWork.type.defaultProps && + !('ref' in finishedWork.memoizedProps) && + !didWarnAboutReassigningProps + ) { if (instance.props !== finishedWork.memoizedProps) { console.error( 'Expected %s props to match memoized props before ' + @@ -856,7 +861,11 @@ function commitClassLayoutLifecycles( // but instead we rely on them being set during last render. // TODO: revisit this when we implement resuming. if (__DEV__) { - if (!finishedWork.type.defaultProps && !didWarnAboutReassigningProps) { + if ( + !finishedWork.type.defaultProps && + !('ref' in finishedWork.memoizedProps) && + !didWarnAboutReassigningProps + ) { if (instance.props !== finishedWork.memoizedProps) { console.error( 'Expected %s props to match memoized props before ' + @@ -913,7 +922,11 @@ function commitClassCallbacks(finishedWork: Fiber) { if (updateQueue !== null) { const instance = finishedWork.stateNode; if (__DEV__) { - if (!finishedWork.type.defaultProps && !didWarnAboutReassigningProps) { + if ( + !finishedWork.type.defaultProps && + !('ref' in finishedWork.memoizedProps) && + !didWarnAboutReassigningProps + ) { if (instance.props !== finishedWork.memoizedProps) { console.error( 'Expected %s props to match memoized props before ' + diff --git a/packages/react-reconciler/src/__tests__/ReactClassComponentPropResolution-test.js b/packages/react-reconciler/src/__tests__/ReactClassComponentPropResolution-test.js new file mode 100644 index 0000000000000..576c6991af065 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactClassComponentPropResolution-test.js @@ -0,0 +1,92 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + */ + +'use strict'; + +let React; +let ReactNoop; +let Scheduler; +let act; +let assertLog; + +describe('ReactClassComponentPropResolution', () => { + beforeEach(() => { + jest.resetModules(); + + React = require('react'); + ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); + act = require('internal-test-utils').act; + assertLog = require('internal-test-utils').assertLog; + }); + + function Text({text}) { + Scheduler.log(text); + return text; + } + + test('resolves ref and default props before calling lifecycle methods', async () => { + const root = ReactNoop.createRoot(); + + function getPropKeys(props) { + return Object.keys(props).join(', '); + } + + class Component extends React.Component { + shouldComponentUpdate(props) { + Scheduler.log( + 'shouldComponentUpdate (prev props): ' + getPropKeys(this.props), + ); + Scheduler.log( + 'shouldComponentUpdate (next props): ' + getPropKeys(props), + ); + return true; + } + componentDidUpdate(props) { + Scheduler.log('componentDidUpdate (prev props): ' + getPropKeys(props)); + Scheduler.log( + 'componentDidUpdate (next props): ' + getPropKeys(this.props), + ); + return true; + } + componentDidMount() { + Scheduler.log('componentDidMount: ' + getPropKeys(this.props)); + return true; + } + render() { + return ; + } + } + + Component.defaultProps = { + default: 'yo', + }; + + // `ref` should never appear as a prop. `default` always should. + + // Mount + const ref = React.createRef(); + await act(async () => { + root.render(); + }); + assertLog(['render: text, default', 'componentDidMount: text, default']); + + // Update + await act(async () => { + root.render(); + }); + assertLog([ + 'shouldComponentUpdate (prev props): text, default', + 'shouldComponentUpdate (next props): text, default', + 'render: text, default', + 'componentDidUpdate (prev props): text, default', + 'componentDidUpdate (next props): text, default', + ]); + }); +}); diff --git a/packages/react/src/__tests__/ReactCreateElement-test.js b/packages/react/src/__tests__/ReactCreateElement-test.js index cf372a33e35f0..c4a2d66310492 100644 --- a/packages/react/src/__tests__/ReactCreateElement-test.js +++ b/packages/react/src/__tests__/ReactCreateElement-test.js @@ -90,7 +90,7 @@ describe('ReactCreateElement', () => { ); }); - // @gate !enableRefAsProp + // @gate !enableRefAsProp || !__DEV__ it('should warn when `ref` is being accessed', async () => { class Child extends React.Component { render() {