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

Remove irrelevant suggestion of a legacy method from a warning #13169

Merged
merged 6 commits into from
Aug 2, 2018

Conversation

Brew-Brew
Copy link
Contributor

@Brew-Brew Brew-Brew commented Jul 7, 2018

As react version is updated and some lifecycle is deprecated, warn message should be changed like in my pr :)

Warning Content:
Cannot update during an existing state transition (such as withinrenderor another component's constructor). Render methods should be a pure function of props and state; constructor side-effects are an anti-pattern, but can be moved to componentWillMount.

What is the expected behavior?
I thought, warning messages what use deprecated lifecycle method should be changed!! :)

'be a pure function of props and state; constructor side-effects are ' +
'an anti-pattern, but can be moved to `componentWillMount`.',
'be a pure function of props and state. ' +
'Move the setState call to componentDidMount`.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is too prescriptive. We don't actually know they called setState in constructor, or that they want to move it to cDM (setting it in constructor might be better in most cases).

Copy link
Contributor Author

@Brew-Brew Brew-Brew Jul 11, 2018

Choose a reason for hiding this comment

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

@gaearon I agree:) Whatever, i thought deprecated lifecycle error message should be deleted! As you say, I will fix pr by setting warn message by deleting message about prescriptive message (constructor and cDM)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gaearon is it ok?

Copy link
Contributor Author

@Brew-Brew Brew-Brew left a comment

Choose a reason for hiding this comment

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

i fixed pr!

'be a pure function of props and state; constructor side-effects are ' +
'an anti-pattern, but can be moved to `componentWillMount`.',
'be a pure function of props and state. ' +
'Move the setState call to componentDidMount`.',
Copy link
Contributor Author

@Brew-Brew Brew-Brew Jul 11, 2018

Choose a reason for hiding this comment

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

@gaearon I agree:) Whatever, i thought deprecated lifecycle error message should be deleted! As you say, I will fix pr by setting warn message by deleting message about prescriptive message (constructor and cDM)

'be a pure function of props and state; constructor side-effects are ' +
'an anti-pattern, but can be moved to `componentWillMount`.',
'be a pure function of props and state. ' +
'Move the setState call to 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.

@gaearon is it ok?

@Brew-Brew Brew-Brew changed the title Edit warn message what use deprecated lifecycle method Edit warn message what use prescriptive and deprecated lifecycle method Jul 11, 2018
@Brew-Brew Brew-Brew changed the title Edit warn message what use prescriptive and deprecated lifecycle method Edit warn message about prescriptive constructor warn and deprecated lifecycle method Jul 11, 2018
@luyaor
Copy link

luyaor commented Jul 19, 2018

Hi @zx6658 ,
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: #12760 which might be a potentially duplicate to this one. We would really appreciate if you could help us to validate and give us feedback.

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.

I think this is right. It seems like this code path only runs for setState in render, and not for constructor. The constructor message is different and is specified by the noop queue in the isomorphic package.

@gaearon gaearon changed the title Edit warn message about prescriptive constructor warn and deprecated lifecycle method Remove irrelevant suggestion of a legacy method from a warning Aug 2, 2018
@gaearon gaearon merged commit 6db0801 into facebook:master Aug 2, 2018
@gaearon gaearon mentioned this pull request Sep 5, 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.

4 participants