Enable warning for defaultProps on function components for everyone #25699
Conversation
|
Comparing: 6fb8133...eff7b63 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
|
|
There's a case, we maybe don't need to warn about. Most defaultProps are deprecated, except on classes. Later we'll move the resolution of defaultProps on classes to deeper in the reconciliation than in createElement. This means that you can read |
These can be tested against the builds now.
sebmarkbage
force-pushed
the
fndefaultprops
branch
4 times, most recently
from
5e60c08 to
0d2bac7
Compare
sebmarkbage
force-pushed
the
fndefaultprops
branch
from
0d2bac7 to
eff7b63
Compare
There was a problem hiding this comment.
Are we not concerned with disabling these feature flags for certain forks in the future? Wouldn't that cause failing tests since they don't conditionally assert on warnings depending on the feature flag values?
If we are, I can follow-up with making sure the assertions check the feature flag values first.
| it('should not warn when owner and self are the same for string refs', () => { | ||
| ReactFeatureFlags.warnAboutStringRefs = false; | ||
|
|
||
| it('should warn when owner and self are the same for string refs', () => { |
There was a problem hiding this comment.
As far as I can tell the added warning is not because of owner and self are the same but simply because string refs are used.
It should still "not warn if owner and self are the same for string refs", right?
I think the test description is fine to keep while also not doing the ReactFeatureFlags.warnAboutStringRefs = false;.
toErrorDev should fail the test if we would suddenly add a warning "when owner and self are the same for string refs" because that would be a different warning than the one we're expecting now?
There was a problem hiding this comment.
The important thing is that it warns since we're about to change behavior. Doesn't really matter all that much for what reason it warns.
|
Not too worried about disabling these since they're going into the next stable and for Meta they're silenced secondarily in a different layer anyway. So I figured we'd just remove the flags as we do a pass over all the ones that we want always on. |
|
I imagine that if defaultProps support is going to be dropped in the next React major release, it is to enable new features. Is there any discussion about this somewhere? |
@icswillems See reactjs/rfcs#107 |
This also fixes a gap where were weren't warning on memo components.
Part of reactjs/rfcs#107: Deprecate
defaultPropson function components