-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Refactor deprecated lifecycles #1515
Conversation
@@ -881,7 +871,7 @@ class Calendar extends React.Component { | |||
getters, | |||
localizer, | |||
viewNames, | |||
} = this.state.context | |||
} = this.getContext(this.props) |
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 won't work as well, The context is in state specifically so that it doesn't change often. Here it will trigger a context update on every render
this._teardownSelectable() | ||
|
||
this.slotMetrics = this.slotMetrics.update(nextProps) | ||
} | ||
this.slotMetrics = this.slotMetrics.update(this.props) |
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 should probably be done in render()
here is a little late
componentDidUpdate() { | ||
if (this.state.needLimitMeasure) this.measureRowLimit(this.props) | ||
componentDidUpdate(prevProps) { | ||
if (!dates.eq(prevProps.date, this.props.date, 'month')) { |
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 sets should be collapsed and the state removed. this check just determines if the measureRowLimit should run, so instead of setting state, do the work instead
componentWillReceiveProps(nextProps) { | ||
const { min, max, timeslots, step } = nextProps | ||
componentDidUpdate() { | ||
const { min, max, timeslots, step } = this.props | ||
this.slotMetrics = this.slotMetrics.update({ min, max, timeslots, step }) |
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 this too late to update this, because it's consumed in render. it needs to update before render
Thanks for submitting this @jetpackjarrett. Good to know that once this is merged the package will be quieter in recent versions and better prepared for React 17. |
We will probably still want changes like the ones in this PR eventually. While #1578 suppresses warnings about the deprecated methods, my understanding is that using “UNSAFE_” methods will still eventually cause compatibility issues with React. |
Since
componentWillMount
andcomponentWillReceiveProps
are deprecated, my implementation was getting a lot of warnings. I went refactored those components to use safe lifecycle methods