-
-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rename componentWillReceiveProps to UNSAFE #716
rename componentWillReceiveProps to UNSAFE #716
Conversation
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.
Thanks @NickFoden!
I think we can switch to componentWillUpdate
in most cases fairly painlessly to make this more future-proof. I can help with that refactoring too if needed
@@ -72,7 +72,7 @@ class IdyllDocument extends React.Component { | |||
this.setState({ error: error.message }); | |||
} | |||
|
|||
componentWillReceiveProps(newProps) { |
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.
Again, we can probably refactor this to use componentDidUpdate as well. This one might be the most work since there's a fair amount of logic in here
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.
Ok yeah I need to step through a few times and then can take a swing at it.
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.
I left this one off latest commit, going to find more idyll projects / build something to get more acquainted
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.
Makes sense - let me know if I can be of assistance. We can hold this one for a separate PR too if its easier.
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.
Yeah exactly, was thinking to separate.
Think this is now ready for review, i would not want to merge yet, until we can confirm the functionality and test few more ways. |
componentWillUpdate(nextProps) { | ||
const { equation } = nextProps; | ||
componentDidUpdate(prevProps) { | ||
const { equation } = prevProps; | ||
// Only instantiate & update the calculator | ||
// when necessary to improve performance. | ||
if ( |
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.
This LGTM
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.
Oops - just found a small issue with this:
when it ends up rendering the equation on line 112
, it uses the equation from prevProps
, but we really want it to use the latest equation
available, otherwise the rendering is always a tick behind
The changes look good to me. I'm going to poke around with it on a local project to test a bit more thoroughly before merging |
componentWillUpdate(nextProps) { | ||
const { equation } = nextProps; | ||
componentDidUpdate(prevProps) { | ||
const { equation } = prevProps; | ||
// Only instantiate & update the calculator | ||
// when necessary to improve performance. | ||
if ( |
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.
Oops - just found a small issue with this:
when it ends up rendering the equation on line 112
, it uses the equation from prevProps
, but we really want it to use the latest equation
available, otherwise the rendering is always a tick behind
Thanks for your patience on this @NickFoden - I found one small bug when testing and commented it above. I'm ready to merge this in once that's resolved |
Hey no worries, and a fast update that still very much on my radar and thinking can/will sort first half of this week. |
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.
Reviewed all files and updated Desmos componentDidUpdate to fix rendering bug.
Took note of idyll-document/src/index.js
for later update in React 17 issue/pr.
Everything looks good to me! Ready to merge if updated Desmos looks ok to you. @mathisonian @NickFoden |
Exciting! I am busy busy today, but can maybe run the code this evening. I think it looks good from fast scan. |
Looks good to me, thanks @megan-vo and @NickFoden! I'm going to go ahead and merge this and include it in the next release |
Rename of the unsafe lifecycle methods as step 1 to get ready for React 17, will have to remove them for 17.
https://reactjs.org/docs/react-component.html#unsafe_componentwillupdate
What is the current behavior? (You can also link to an open issue here)
React lifecycle warning #711
What is the new behavior (if this is a feature change)?
Less warnings in console.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No
Other information:
Longer term fix is to refactor some of these methods either into hooks or other existing "safe" methods