Remove defaultProps support (except for classes) #28733
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
facebook-github-bot
added
CLA Signed
React Core Team
acdlite
commented
Apr 3, 2024
| }); | ||
| }, | ||
| }, | ||
| propTypes: { |
There was a problem hiding this comment.
Unrelated, but I removed this warning entirely since propTypes support has already been removed from the codebase.
This removes defaultProps support for all component types except for
classes. We've chosen to continue supporting defaultProps for classes
because lots of older code relies on it, and unlike function components,
(which can use default params), there's no straightforward alternative.
By implication, it also removes support for setting defaultProps on
`React.lazy` wrapper. So this will not work:
```js
const MyClassComponent = React.lazy(() => import('./MyClassComponent'));
// MyClassComponent is not actually a class; it's a lazy wrapper. So
// defaultProps does not work.
MyClassComponent.defaultProps = { foo: 'bar' };
```
However, if you set the default props on the class itself, then
it's fine.
For classes, this change also moves where defaultProps are resolved.
Previously, defaultProps were resolved by the JSX runtime. This change
is only observable if you introspect a JSX element, which is relatively
rare but does happen.
In other words, previously `<ClassWithDefaultProp />.props.aDefaultProp`
would resolve to the default prop value, but now it does not.
acdlite
force-pushed
the
remove-defaultprops-support
branch
from
44a5b6c
to
41b4a69
Compare
sebmarkbage
reviewed
Apr 4, 2024
| if (props[propName] === undefined) { | ||
| props[propName] = defaultProps[propName]; | ||
| if (!disableDefaultPropsExceptForClasses) { | ||
| // Resolve default props |
There was a problem hiding this comment.
Shouldn't this be removed even when the flag is off? Because the resolution moves regardless?
There was a problem hiding this comment.
The way I implemented the flag is that when it's off, it maintains the old behavior: props resolved during JSX. I did it that way to aid the roll out at Meta and reduce the risk the whole change gets reverted. Since there could be code that reads element.props and expects the defaultProps to already be there.
There was a problem hiding this comment.
So really the flag handles both moving the resolution and removing it for non-classes, I just didn't think it was worth creating two separate flags.
sebmarkbage
approved these changes
Apr 4, 2024
github-actions bot
pushed a commit
that referenced
this pull request
Apr 4, 2024
This removes defaultProps support for all component types except for
classes. We've chosen to continue supporting defaultProps for classes
because lots of older code relies on it, and unlike function components,
(which can use default params), there's no straightforward alternative.
By implication, it also removes support for setting defaultProps on
`React.lazy` wrapper. So this will not work:
```js
const MyClassComponent = React.lazy(() => import('./MyClassComponent'));
// MyClassComponent is not actually a class; it's a lazy wrapper. So
// defaultProps does not work.
MyClassComponent.defaultProps = { foo: 'bar' };
```
However, if you set the default props on the class itself, then it's
fine.
For classes, this change also moves where defaultProps are resolved.
Previously, defaultProps were resolved by the JSX runtime. This change
is only observable if you introspect a JSX element, which is relatively
rare but does happen.
In other words, previously `<ClassWithDefaultProp />.props.aDefaultProp`
would resolve to the default prop value, but now it does not.
DiffTrain build for [48b4ecc](48b4ecc)
github-actions bot
pushed a commit
that referenced
this pull request
Apr 11, 2024
This removes defaultProps support for all component types except for
classes. We've chosen to continue supporting defaultProps for classes
because lots of older code relies on it, and unlike function components,
(which can use default params), there's no straightforward alternative.
By implication, it also removes support for setting defaultProps on
`React.lazy` wrapper. So this will not work:
```js
const MyClassComponent = React.lazy(() => import('./MyClassComponent'));
// MyClassComponent is not actually a class; it's a lazy wrapper. So
// defaultProps does not work.
MyClassComponent.defaultProps = { foo: 'bar' };
```
However, if you set the default props on the class itself, then it's
fine.
For classes, this change also moves where defaultProps are resolved.
Previously, defaultProps were resolved by the JSX runtime. This change
is only observable if you introspect a JSX element, which is relatively
rare but does happen.
In other words, previously `<ClassWithDefaultProp />.props.aDefaultProp`
would resolve to the default prop value, but now it does not.
DiffTrain build for [48b4ecc](48b4ecc)
github-actions bot
pushed a commit
that referenced
this pull request
Apr 11, 2024
This removes defaultProps support for all component types except for
classes. We've chosen to continue supporting defaultProps for classes
because lots of older code relies on it, and unlike function components,
(which can use default params), there's no straightforward alternative.
By implication, it also removes support for setting defaultProps on
`React.lazy` wrapper. So this will not work:
```js
const MyClassComponent = React.lazy(() => import('./MyClassComponent'));
// MyClassComponent is not actually a class; it's a lazy wrapper. So
// defaultProps does not work.
MyClassComponent.defaultProps = { foo: 'bar' };
```
However, if you set the default props on the class itself, then it's
fine.
For classes, this change also moves where defaultProps are resolved.
Previously, defaultProps were resolved by the JSX runtime. This change
is only observable if you introspect a JSX element, which is relatively
rare but does happen.
In other words, previously `<ClassWithDefaultProp />.props.aDefaultProp`
would resolve to the default prop value, but now it does not.
DiffTrain build for [48b4ecc](48b4ecc)
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
This removes defaultProps support for all component types except for
classes. We've chosen to continue supporting defaultProps for classes
because lots of older code relies on it, and unlike function components,
(which can use default params), there's no straightforward alternative.
By implication, it also removes support for setting defaultProps on
`React.lazy` wrapper. So this will not work:
```js
const MyClassComponent = React.lazy(() => import('./MyClassComponent'));
// MyClassComponent is not actually a class; it's a lazy wrapper. So
// defaultProps does not work.
MyClassComponent.defaultProps = { foo: 'bar' };
```
However, if you set the default props on the class itself, then it's
fine.
For classes, this change also moves where defaultProps are resolved.
Previously, defaultProps were resolved by the JSX runtime. This change
is only observable if you introspect a JSX element, which is relatively
rare but does happen.
In other words, previously `<ClassWithDefaultProp />.props.aDefaultProp`
would resolve to the default prop value, but now it does not.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
This removes defaultProps support for all component types except for
classes. We've chosen to continue supporting defaultProps for classes
because lots of older code relies on it, and unlike function components,
(which can use default params), there's no straightforward alternative.
By implication, it also removes support for setting defaultProps on
`React.lazy` wrapper. So this will not work:
```js
const MyClassComponent = React.lazy(() => import('./MyClassComponent'));
// MyClassComponent is not actually a class; it's a lazy wrapper. So
// defaultProps does not work.
MyClassComponent.defaultProps = { foo: 'bar' };
```
However, if you set the default props on the class itself, then it's
fine.
For classes, this change also moves where defaultProps are resolved.
Previously, defaultProps were resolved by the JSX runtime. This change
is only observable if you introspect a JSX element, which is relatively
rare but does happen.
In other words, previously `<ClassWithDefaultProp />.props.aDefaultProp`
would resolve to the default prop value, but now it does not.
DiffTrain build for commit 48b4ecc.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
This removes defaultProps support for all component types except for
classes. We've chosen to continue supporting defaultProps for classes
because lots of older code relies on it, and unlike function components,
(which can use default params), there's no straightforward alternative.
By implication, it also removes support for setting defaultProps on
`React.lazy` wrapper. So this will not work:
```js
const MyClassComponent = React.lazy(() => import('./MyClassComponent'));
// MyClassComponent is not actually a class; it's a lazy wrapper. So
// defaultProps does not work.
MyClassComponent.defaultProps = { foo: 'bar' };
```
However, if you set the default props on the class itself, then it's
fine.
For classes, this change also moves where defaultProps are resolved.
Previously, defaultProps were resolved by the JSX runtime. This change
is only observable if you introspect a JSX element, which is relatively
rare but does happen.
In other words, previously `<ClassWithDefaultProp />.props.aDefaultProp`
would resolve to the default prop value, but now it does not.
DiffTrain build for commit 48b4ecc.
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.
This removes defaultProps support for all component types except for classes. We've chosen to continue supporting defaultProps for classes because lots of older code relies on it, and unlike function components, (which can use default params), there's no straightforward alternative.
By implication, it also removes support for setting defaultProps on
React.lazywrapper. So this will not work:However, if you set the default props on the class itself, then it's fine.
For classes, this change also moves where defaultProps are resolved. Previously, defaultProps were resolved by the JSX runtime. This change is only observable if you introspect a JSX element, which is relatively rare but does happen.
In other words, previously
<ClassWithDefaultProp />.props.aDefaultPropwould resolve to the default prop value, but now it does not.