-
Notifications
You must be signed in to change notification settings - Fork 319
Replace use of deprecated lifecycle method componentWillReceiveProps #255
base: master
Are you sure you want to change the base?
Conversation
@rynobax thanks for this PR! On a cursory look, it seems like a change worth making. Can you look into the CI error and get things green so we can do a full review? Thanks! |
CI is passing now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rynobax thanks for the PR! These changes look good to me.
That said, I think they'll need to wait for a new major version to merge.
While the movement of logic between lifecycle methods is probably safe for most apps, it's possible that someone with a particularly complex usage could be relying on the exact order in which these things happen. (That an element's DOM is already mutated before render
on the parent component is called, for example.)
👍 |
@rynobax you planning on fixing the simple conflict so this can get merged? |
75cc4b6
to
eb44361
Compare
Rebased the branch and let yarn fix the conflict |
Thank you for updating this PR! Unfortunately we are no closer to a major version release than when this PR was opened. We will keep this in mind for the future when other breaking changes are introduced. (Happy to take a separate PR for the enzyme changes though if you'd like) |
Hi, now Rect 16.9 deprecates componentWillReceiveProps, it would be useful to merge this! |
@yohannprigent: good point! Looks like I need to rebase and review this, since it's a bit old, but we should get 16.9 support working soon. |
A fix for this is now in master and we will cut a 5.0 release shortly, so going to close. Thanks for the PR and sorry we took so long to get around to this! |
|
Summary & motivation
When rendered in React Strict mode, this library generates warnings because it is using the deprecated lifecycle method
componentWillReceiveProps
. I moved the code inside ofcomponentWillReceiveProps
intocomponentDidUpdate
, as suggested in this React blog post.Testing & documentation
The changes to the the code needed to remove cWRP was pretty straightforward. However, Enzyme v2 does not call cDU, so I migrated the tests to enzyme v3 in order to get them to pass. There are other ways to keep the tests passing without upgrading Enzyme (namely switching from shallow rendering to full mounting), but this seemed the simplest.
I also fixed a bug in the demo page that was exposed by strict mode, where a request was being fired in the constructor, and the handler was running before the component had been mounted (which threw an error). Moving the request into componentDidMount fixed it.