-
Notifications
You must be signed in to change notification settings - Fork 288
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
Remove unsafe react lifecycle hooks (Fixes #492) #509
Conversation
Update: running |
Awesome, thanks for putting this together, and apologies for our delays responding. This looks good to me. @pradeepnschrodinger could you take a look at well? I have 2 concerns which I think we should explore before approving for merge.
|
Thanks for the review @wcjordan. I think I've fixed the infinite loop issue. It sounds like the React 13/14 issue is beyond the scope of this PR so I'll leave it to you and @pradeepnschrodinger to weigh in on the right approach there. |
Awesome, thank you. Niranjan & I will work out dealing w/ React 13 & 14 and get back to you shortly on merging this. |
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.
Looks good to me
src/FixedDataTableCell.js
Outdated
@@ -125,7 +125,10 @@ class FixedDataTableCell extends React.Component { | |||
return false; | |||
} | |||
|
|||
componentWillReceiveProps(props) { | |||
componentDidUpdate(prevProps) { | |||
if (prevProps === this.props) return; |
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.
Is this only true when the component gets rerendered due to a state change within itself?
Also can you reformat this to:
if (prevProps === this.props) {
return;
}
In the long term, can we skip the extra rerender through use of getDerivedStateFromProps
? @wcjordan
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.
Also, since getDerivedStateFromProps
was added in React v16.3, will this mean we'll deprecate every react version under v16.3.
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.
Yuck, we don't want to deprecate support for everything under 16.3. @pradeepnschrodinger could you ticket out the idea of using getDerivedStateFromProps and we can discuss when may make sense to do that on our roadmap?
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.
Created #512
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.
Great, thanks
src/FixedDataTableContainer.js
Outdated
componentWillReceiveProps(nextProps) { | ||
componentDidUpdate(prevProps) { | ||
// Only react to prop changes - ignore updates due to internal state | ||
if (this.props === prevProps) return; |
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.
reformat to
if (this.props === prevProps) {
return;
}
|
||
this.setState(boundState); | ||
update() { | ||
this.setState(this.getBoundState()); |
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. Not sure how this.setState
worked in the constructor earlier.
@boxfoot could you update the package.json peerDependencies for react & react-dom to require "^15.3.0"? I've created a v1.0.x branch which will use if we patch anything else to v1.0 and we'll target master for a v1.1 release where we drop support for React 13, 14, & early 15 (FDT has already stopped working with those versions given our use of React.PureComponent...) |
@wcjordan should it be |
Good call, now that react is using the major version number ^ isn't enough. Let's do ">=15.3.0" |
4413526
to
d28f9cd
Compare
All changes committed (and squashed) |
Sweet, thanks. I've merged to mainline and we'll release w/ v1.1.0 |
We've changed few parts seen in this PR as it had led to some bugs with one of our applications. We also believe that it leads to breakages for other users. See the cause of the bug and explanation, along with the full changes in PR: #523. |
Thanks for the update, good to know! |
This is now released w/ v1.1. |
Removes all use of
componentWillMount
andcomponentWillReceive Props
Motivation and Context
As discussed in #492 -- React will be deprecating some of its lifecycle methods in the next major version. https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html
How Has This Been Tested?
All unit tests pass, and all of the examples in the sample site also work as expected.
Types of changes
Checklist: