-
Notifications
You must be signed in to change notification settings - Fork 47.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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 <jan@kassens.net>- Loading branch information
Showing
6 changed files
with
123 additions
and
21 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
92 changes: 92 additions & 0 deletions
92
packages/react-reconciler/src/__tests__/ReactClassComponentPropResolution-test.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 <Text text={'render: ' + getPropKeys(this.props)} />; | ||
| } | ||
| } | ||
|
|
||
| Component.defaultProps = { | ||
| default: 'yo', | ||
| }; | ||
|
|
||
| // `ref` should never appear as a prop. `default` always should. | ||
|
|
||
| // Mount | ||
| const ref = React.createRef(); | ||
| await act(async () => { | ||
| root.render(<Component text="Yay" ref={ref} />); | ||
| }); | ||
| assertLog(['render: text, default', 'componentDidMount: text, default']); | ||
|
|
||
| // Update | ||
| await act(async () => { | ||
| root.render(<Component text="Yay (again)" ref={ref} />); | ||
| }); | ||
| assertLog([ | ||
| 'shouldComponentUpdate (prev props): text, default', | ||
| 'shouldComponentUpdate (next props): text, default', | ||
| 'render: text, default', | ||
| 'componentDidUpdate (prev props): text, default', | ||
| 'componentDidUpdate (next props): text, default', | ||
| ]); | ||
| }); | ||
| }); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters