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

"react/no-did-mount-set-state", not considered an valid rule since React 16.3.0 #1754

Closed
bumpah opened this issue Apr 1, 2018 · 17 comments
Closed

Comments

@bumpah
Copy link

bumpah commented Apr 1, 2018

Not valid rule on latest React -release, should be removed.

@bumpah bumpah changed the title eslint-plugin-react/docs/rules/no-did-mount-set-state "react/no-did-mount-set-state", not considered an valid rule since React 16.3.0 Apr 1, 2018
@ljharb
Copy link
Member

ljharb commented Apr 1, 2018

Can you elaborate?

@bumpah
Copy link
Author

bumpah commented Apr 2, 2018

Sorry for not been to clear. "componentWillMount", "componentWillUpdate", "componentWillReceiveProps" will be deprecated life-cycle methods (React 16.3.0, introduced these and deprecation warnings are scheduled to come with 16.4.0), and replacements for those will be accordingly "componentWillMount" => "componentDidMount", "componentWillUpdate"  => "componentDidUpdate" and "componentWillReceiveProps" => with new introduced "getDerivedStateFromProps".

Therefore it's not relevant to have this rule and existing code bases should be migrated soon enough to match new life-cycles. https://medium.com/@baphemot/whats-new-in-react-16-3-d2c9b7b6193b here's short Medium posting according to changes.

@ljharb
Copy link
Member

ljharb commented Apr 2, 2018

I'm aware of those deprecations, but I'm not sure why it suddenly means that componentDidMount - which is NOT deprecated - would be able to have a setState call in it.

@teameh
Copy link
Contributor

teameh commented Apr 3, 2018

Since componentWillMount will disappear the state change done needs to go somewhere else. For this case, users can almost always use getDerivedStateFromProps to achieve this, but there will be some other cases where users need to call setState in componentDidMount or componentDidUpdate.

From the blogpost:

{...} In general, it is better to avoid cascading updates like this, but in some cases they are necessary (for example, if you need to position a tooltip after measuring the rendered DOM element).

Whole post with more examples and best practices: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html

@bumpah
Copy link
Author

bumpah commented Apr 3, 2018

In my opinion most relevant use cases for this, would be async rendering or if you would like to meddle with refs.

I don't see what is issue having setState in componentDidMount, except if you consider extra render as performance issue.

@ljharb
Copy link
Member

ljharb commented Apr 4, 2018

An extra render is definitely a performance issue; but an extra render with different state causes a user-visible flash.

@kentcdodds
Copy link
Contributor

kentcdodds commented Apr 4, 2018

an extra render with different state causes a user-visible flash.

This is inaccurate. According to "Update on Async Rendering" on React's blog:

Sometimes people use componentWillUpdate out of a misplaced fear that by the time componentDidUpdate fires, it is “too late” to update the state of other components. This is not the case. React ensures that any setState calls that happen during componentDidMount and componentDidUpdate are flushed before the user sees the updated UI. In general, it is better to avoid cascading updates like this, but in some cases they are necessary (for example, if you need to position a tooltip after measuring the rendered DOM element).

@kentcdodds
Copy link
Contributor

I believe this applies to both didMount and didUpdate

@ljharb
Copy link
Member

ljharb commented Apr 5, 2018

@kentcdodds thanks for the clarification. do you then agree with the OP, that as of v16.3, these two rules no longer have value?

@kentcdodds
Copy link
Contributor

Yes I do 👍

@ljharb
Copy link
Member

ljharb commented Apr 10, 2018

Seems like the actionable thing to do here is, when the React version pragma is >= v16.3, these rules should no-op.

@yannickcr
Copy link
Member

In general, it is better to avoid cascading updates like this, but in some cases they are necessary (for example, if you need to position a tooltip after measuring the rendered DOM element).

Mmm, it is exactly for this reason that this rule was created in the first place (but was removed from the recommended configuration since there is some legitimate use cases, see #596).

@steida
Copy link

steida commented Jul 26, 2018

Why is this rule not removed? It confused me, but it's perfectly valid for tooltips.

@ljharb
Copy link
Member

ljharb commented Jul 26, 2018

@steida people still use Reacts older than 16.3.

gjvoosten added a commit to NCI-Agency/anet that referenced this issue Apr 26, 2019
We use React >= 16.3, and everywhere we use setState in componentDidUpdate, we do that inside an if where we check against previous values. So the rule does not really apply to us.
See e.g. the discussion here: jsx-eslint/eslint-plugin-react#1754
gjvoosten added a commit to NCI-Agency/anet that referenced this issue Apr 26, 2019
We use React >= 16.3, and everywhere we use setState in componentDidUpdate, we do that inside an if where we check against previous values. So the rule does not really apply to us.
See e.g. the discussion here: jsx-eslint/eslint-plugin-react#1754
@gaborluk
Copy link

gaborluk commented Nov 22, 2019

Seems like the actionable thing to do here is, when the React version pragma is >= v16.3, these rules should no-op.

@yannickcr, @ljharb, has this no-op behavior been implemented? I'm having trouble with ESLint still complaining about this rule, even if I explicitly set settings.react.version to 16.8 in .eslintrc.json.

@ljharb
Copy link
Member

ljharb commented Nov 22, 2019

No, it hasn't yet.

@ljharb ljharb reopened this Nov 22, 2019
@psabharwal123

This comment was marked as spam.

ljharb added a commit to ljharb/eslint-plugin-react that referenced this issue Feb 5, 2022
@ljharb ljharb closed this as completed in 42a8093 Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants