Skip to content
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

chore(multi): Remove use of deprecated lifecycle methods #94

Conversation

ivoreis
Copy link
Contributor

@ivoreis ivoreis commented Apr 23, 2019

React will deprecate a few lifecycle methods and componentWillMount is one of them:
https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#gradual-migration-path

Two possible ways to solve this:

  1. Replace componentWillMount with UNSAFE_componentWillMount
  2. Replace componentWillMount with componentDidMount

This PR removes use of deprecated componentWillMount method and replaces with componentDidMount + constructor

- Remove use of deprecated componentWillMount
@ivoreis
Copy link
Contributor Author

ivoreis commented Apr 23, 2019

👋 @davidtheclark. I've created this PR to prevent React warnings and errors in the upcoming versions. Please let me know what you think about this approach and if you have any concern or suggestion.

Copy link
Contributor

@vutran vutran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏👏👏

@@ -17,7 +17,7 @@ module.exports = class extends React.Component {
ambManager: PropTypes.object.isRequired
};

componentWillMount() {
componentDidMount() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use the constructor, instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you. I'm not very familiar with this codebase and I decided to update this based on React's recommendation. ATM this will only be set on the client since this lifecycle method doesn't run on the server.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can move this to constructor. @ivoreis any specific reason for it to go inside componentDidMount ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR goal is to update the code to be compatible with later react version. Can @davidtheclark provide more context why this wasn’t set in the constructor but in componentWillMount instead in the first place?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidtheclark Any update on this ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of any reason.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivoreis Let's move this inside constructor ?

@shirshendubhowmick
Copy link
Collaborator

shirshendubhowmick commented Sep 1, 2019

@davidtheclark I am planning to merge this PR after verifying all the functionalties. Can create a seperate PR for whatever refactoring needed.
This is because React 16.9 already started giving warning for deperecated lifecycle methods and it will be permanently removed in React 17, so this is a breaking change.

Will let you once everything is merged, so we will be needing a release after that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants