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

Warning message no longer recommends using deprecated lifecycle method. #12760

Closed
wants to merge 5 commits into from

Conversation

iliran11
Copy link

@iliran11 iliran11 commented May 7, 2018

Fixing the folllwing issue: #12748;

  1. componentWillMount is soon to be deprecated method, so recommending to use it, is not a viable options.

  2. Author of the warning message has warned against constructor side-effects. as per the official react docs, the right way to do it, is placing those side effects in componentDidMount.

Avoid introducing any side-effects or subscriptions in the constructor. For those use cases, use componentDidMount() instead.

@iliran11 iliran11 changed the title warning message is now align with new lifecycle paradigm Warning message no longer recommends using deprecated lifecycle method. May 7, 2018
@gaearon
Copy link
Collaborator

gaearon commented May 15, 2018

I'd prefer a warning that's a bit more detailed. "Either assign the initial state in constructor, or move the setState call to componentDidMount."

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

See above

@iliran11
Copy link
Author

iliran11 commented Jun 6, 2018

@gaearon I have updated the warning message. please take a lot.
sorry for the delay by the way :)

@gaearon
Copy link
Collaborator

gaearon commented Jun 6, 2018

Please run yarn prettier and commit

@@ -425,10 +425,9 @@ describe('ReactCompositeComponent', () => {
expect(() => {
instance = ReactDOM.render(<Component />, container);
}).toWarnDev(
'Cannot update during an existing state transition (such as within ' +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add "(such as within render...)" part back? I don't see why it needs to be changed.

Copy link
Author

Choose a reason for hiding this comment

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

was just thinking it was lengthy.
maybe perhaps instead of printing "Cannot update during an existing state transition" , It is better to be concise and print "Cannot update state during render lifecycle method" ?

@gaearon
Copy link
Collaborator

gaearon commented Jun 6, 2018

Looking at this again, I don't see why the message assumes we're in the constructor. This code is for setState in render, isn't it?

Copy link
Author

@iliran11 iliran11 left a comment

Choose a reason for hiding this comment

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

@gaearon why the message assumes we are in the constructor? right now it suggests to initially set the state in the constructor, or use setState on componentDidMount , instead of using setState in render (so it assumes we are in render method)

@@ -425,10 +425,9 @@ describe('ReactCompositeComponent', () => {
expect(() => {
instance = ReactDOM.render(<Component />, container);
}).toWarnDev(
'Cannot update during an existing state transition (such as within ' +
Copy link
Author

Choose a reason for hiding this comment

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

was just thinking it was lengthy.
maybe perhaps instead of printing "Cannot update during an existing state transition" , It is better to be concise and print "Cannot update state during render lifecycle method" ?

@iliran11
Copy link
Author

iliran11 commented Jun 6, 2018

I can't really understand why CircleCi is failing on Test Packages job.
it writes in the job output:

Test Suites: 149 passed, 149 total
Tests: 26 skipped, 3043 passed, 3069 total
Snapshots: 40 passed, 40 total
Time: 128.754s
Ran all test suites.
Done in 130.43s.

@aweary
Copy link
Contributor

aweary commented Jun 6, 2018

@iliran11 you need to run yarn prettier

@iliran11
Copy link
Author

iliran11 commented Jun 6, 2018

@aweary @gaearon OK, the CI has passed finally. yarn prettier didn't do the trick. it made no changes to files. git status returned nothing to commit.

in the end i used prettier plugin in visual code to solve this and just formatted the files I edited.

@luyaor
Copy link

luyaor commented Jul 19, 2018

Hi Liran Cohen(@iliran11),
We are researchers working on identifying redundant development and duplicated pull requests. We would like to help open source community to reduce redundant work. We have found there is another open pull request: #13169 which might be a potentially duplicate to this one. We would really appreciate if you could help us to validate and give us feedback.

@gaearon Sorry again if it creates notification noise on the repository.

@gaearon
Copy link
Collaborator

gaearon commented Aug 2, 2018

Sorry for the churn. I ended up merging #13169 because it was more accurate: as it turned out, this code path doesn't actually run for the constructor anymore. So mentioning constructor didn't make sense. Thanks for the effort though!

@gaearon gaearon closed this Aug 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants