Eslint-plugin-react: I think we should remove `no-did-mount-set-state` and `no-did-update-set-state` rules

[renatorib]

According to the react guys this is not wrong, neither bad practice.

Can be seen at the following links:

• https://github.com/reactjs/reactjs.org/pull/596#discussion_r171357647
• https://twitter.com/acdlite/status/968938221852753921
• https://twitter.com/dan_abramov/status/968939189709533184

I thought about sending a PR, but I do not know how eslint works, I do not know if it's just delete or deprecate. So I just created the issue.

[ljharb]

Using setState directly inside didMount or didUpdate might be fine, but if you use it inside didMount or didUpdate after an async action, then it will definitely cause problems (nothing in those links disagrees with this).

[renatorib]

I got it. Thanks.

[Tomekmularczyk]

I think this error is still a bit confusing. For example, this gives me an error while it supposed to be just fine:

  componentDidUpdate(prevProps) {
    const finished = prevProps.isFetching && !this.props.isFetching;

    if (finished && !this.props.error) {
      this.setState({ email: '', sent: true });
    }
  }

After componentWillReceiveProps has been deprecated I think there will be a lot of situations like this.

[ljharb]

@Tomekmularczyk the proper way to do that is now to use getDerivedStateFromProps, i believe.

[Tomekmularczyk]

@ljharb unless you need to have access to previous props, which is not possible with getDerivedStateFromProps. It would require saving some props in a state for comparisons. For simple situations like above componentDidUpdate is simpler option knowing that update like that is just fine.
But I'm not gonna speak for everyone else because of I just started migrating the code and I might be missing something.

[nishmeht7]

@ljharb what is the status of this?

React docs mentions that it is alright to call setState in componentDidUpdate as long as it's inside an if statement

"You may call setState() immediately in componentDidUpdate() but note that it must be wrapped in a condition ..."

[AlmogRnD]

Is there any news about this?

[wanaryytel]

I think this should be reopened.

[renatorib]

Reopened issue for having more people thinking the same

[tastypackets]

I agree this rule should be removed, according to the React docs it is okay to setState inside componentDidMount

You may call setState() immediately in componentDidMount(). It will trigger an extra rendering, but it will happen before the browser updates the screen. This guarantees that even though the render() will be called twice in this case, the user won’t see the intermediate state. Use this pattern with caution because it often causes performance issues. In most cases, you should be able to assign the initial state in the constructor() instead. It can, however, be necessary for cases like modals and tooltips when you need to measure a DOM node before rendering something that depends on its size or position.

[ljharb]

no-did-mount-set-state isn't compatible with server-rendering anyways, so i'm content to mark that rule as deprecated.

@tastypackets your quoted text doesn't related to cDU, however.

As for no-did-update-set-state, if the rule can be modified to ensure the safe conditions the React docs mention, let's do so; otherwise, it's better to err on the side of over-prohibition.

[tastypackets]

Sorry that was a typo, I meant to write didMount. I'll edit the post. 🙂

With the use of hooks I haven't run into this again tbh.

[emilenriquez]

it would be nice if the rule would ensure safe usage of setState insude cDU. otherwise IMHO it might be good if the rule will be changed to something like setState without parent if condition or something of the like. :)

[muthu1712]

I would agree with @emilenriquez. I personally don't like to write "eslint disable" lines in my code, even after writing a proper code as per the original documentation of React.

[orlandovallejos]

Any news about this?

[likerRr]

okay, any news?