changing name of deprecated react lifecycle methods #2059
Conversation
There was a problem hiding this comment.
You'll also want to update .travis.yml so it stops testing on React 14 and 15, and start testing on React 17.
The latter means this is blocked on enzymejs/enzyme#2430 - so another alternative is requiring ^16.3 and waiting until a future PR to add (in a semver-minor) v17 support.
package.json
Outdated
| "react": "^16.3.0 || ^17.0.0", | ||
| "react-dom": "^16.3.0 || ^17.0.0", |
There was a problem hiding this comment.
just calling out that this makes it a breaking change.
| @@ -109,7 +109,7 @@ class CalendarMonth extends React.PureComponent { | |||
| this.setMonthTitleHeightTimeout = setTimeout(this.setMonthTitleHeight, 0); | |||
| } | |||
|
|
|||
| componentWillReceiveProps(nextProps) { | |||
| UNSAFE_componentWillReceiveProps(nextProps) { | |||
There was a problem hiding this comment.
if we're doing the breaking change, we might as well migrate to getDerivedStateFromProps where we can? It's ok to keep the unsafe versions where that's not easy tho.
There was a problem hiding this comment.
I have a few thoughts on this:
- I want to rock the boat with this PR as little as possible. If people are upgrading from an older react to 16.3+, then upgrading this, if they have a bug it would be nice to say fairly confidently that it wasn't this and probably part of some other part of their migration.
- Related, a ton of the logic in these components is in these lifecycle methods, so I see any substantive change to them as a larger-scope change.
- From my understanding, these methods aren't going anywhere anytime soon. My understanding is the react team has great interest in discouraging their use rather than removing them.
- If you think this is the time/PR to do this lmk and I can see what I can do. Might be more appropriate to do it in two parts.
- Depending on what the future holds, this lib might go the way of many of the other libs out there and get a functional, hooks-based refactor. Maybe instead of migrating all this huge lifecycle method logic twice we just wait until that day comes (though I believe the migration to
getDerivedStateFromPropsis not nearly on the same scale as a migration to hooks, which would be...non-trivial.)
I will say in general I'm all about the "while we're at it" mentality. If it were my own repo or wasn't 427,576 weekly downloads then I'd feel more comfortable just doing it 😂 .
My vote is to do it this dumb, rename-only way first to get the warnings out and get the compatibility there, then either have a follow-up PR or include this in future refactoring conversations/planning.
There was a problem hiding this comment.
If we're going to migrate to hooks any time soon, we'd need to require React v16.9 now, and not just v16.3, even if we do this PR as-is first, and change other things later.
There was a problem hiding this comment.
Yeah definitely not advocating hooks migration in the near-term.
ljharb
added
the
semver-major: breaking change
|
@ljharb just removed < 16 from |
ranneyd
force-pushed
the
unsafe-componentwillreceiveprops
branch
from
2631bac
to
e171106
Compare
|
Is this still blocked? Any way a contributor could help? react-dates is preventing migration to React 17 for me. |
|
Yes; this still needs rebasing, but also would still be a breaking change. |
This comment has been minimized.
This comment has been minimized.
#1748
getDerivedStateFromProps, I figured using theUNSAFEversions of the functions was less disruptive. Perhaps the lib will move to a functional/hooks-based implementation some day.UNSAFEprefix on the function names upset eslint'scamelcaserule. I copied the config from here but added theallowproperty