Fix: Class components should "consume" ref prop #28719
Merged
Conversation
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
|
Comparing: 8f55a6a...bc1fac0 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show |
ec53821
to
a550570
Compare
kassens
approved these changes
Apr 2, 2024
| const prevState = current.memoizedState; | ||
| // We could update instance props and state here, | ||
| // but instead we rely on them being set during last render. | ||
| // TODO: revisit this when we implement resuming. | ||
| if (__DEV__) { | ||
| if ( | ||
| finishedWork.type === finishedWork.elementType && | ||
| !finishedWork.type.defaultProps && |
There was a problem hiding this comment.
This disables the re-assign props warning when defaultProps are present? Seems fine.
72e8cf4
to
6450fc6
Compare
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. As a first step, this adds a new function, resolveClassComponentProps, where both default prop resolution and ref prop removal will happen.
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 <[email protected]>
Noticed these once I enabled class prop resolution in more places. Technically this was already observable if you wrapped a class with `React.lazy` and gave that wrapper default props, but since that was so rare it was never reported.
6450fc6
to
bc1fac0
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Apr 3, 2024
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 <[email protected]>
DiffTrain build for [dc545c8](dc545c8)
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Apr 3, 2024
Same as facebook#28719 but for SSR.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Apr 3, 2024
Same as facebook#28719 but for SSR.
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
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 facebook#28602
---------
Co-authored-by: Jan Kassens <[email protected]>
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
Same as facebook#28719 but for SSR.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
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 <[email protected]>
DiffTrain build for commit dc545c8.
This was referenced Apr 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.propsonto 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.lazythat resolves to a class component).In all of these same places, we will also need to remove the ref prop when
enableRefAsPropis on.Closes #28602