no-did-mount-set-state with arrow functions & React.Component?

[fohrloop]

Hello,

If I am correct, using no-did-mount-set-state should trigger with the following code:

import React from 'react'

class Test extends React.Component {
  state = {}

  componentDidMount = () => {
    this.setState({
      name: this.props.name.toUpperCase()
    })
  }

  render() {
    return <div>Hello {this.state.name}</div>
  }
}

export default Test

For some reason, I do not get any errors from ESLint. The actual rule I am using is 'react/no-did-mount-set-state': [2, 'disallow-in-func'].

Question

Should the rule trigger with the code above?

System setup

Windows 10, node v.8.9.3, eslint-plugin-react v. 7.5.1, ESLint v.3.19.0 & babel-eslint 8.0.3.

[ljharb]

This seems related to #1597

[jaaberg]

You are correct, your code should trigger this rule.

The problem:
This rule looks for CallExpressions of type this.setState, and for all this.setState's, we check all the ancestor nodes by three things:

1. The node is either a Property or a MethodDefinition
2. The name of the Property/Method is componentDidMount
3. If setState is called directly within componentDidMount or is a part of some function-callback

In your example componentDidMount is declared as an AssignmentExpression and not a Property or a MethodDefinition. This is why it is not triggered, and we need to check for that as well.

If someone wants to make a contribution, please feel free, or else I will have a look at it when I find time.

[ljharb]

(It's actually a really bad idea to put functions in class properties; but the rule should still handle it)

[fohrloop]

@ljharb Could you please elaborate on that? If you mean the usage of arrow function to attach a function to a class, what do you think about this?

[ljharb]

@np-8 first of all, React lifecycle methods never need to be bound to the class, so it's just wasting performance and clarity. That linked article talks about event handlers only. The article's example of constructor-bindings for instance methods is in fact the best possible solution (based on stage 3 proposals and above); using arrow functions in class properties isn't equivalent (for testing, performance, or clarity).

[fohrloop]

Thanks. I was not aware that the lifecycle methods behave differently. I though any method could be bound to React.Component using the arrow function notation. I come from Python where there is clear distinction between classes and instances. To me, in JS everything looks the same (not sure if the methods are for the React.Component class or instances of that class).

[ljharb]

Any method can be bound. The issue is whether it has to be, because in JS, the this depends on how it's called as well as how it's defined. React lifecycle methods are always called chained off the component instance, so the this never needs to be pre-set.

[fohrloop]

Yeah, I understand. So finding this "bug" is not neccessarily very useful :) Thanks for the clarifications!

[ljharb]

It's still useful for people who misuse class properties in this way :-) just not for the specific use case you found. Thanks for reporting!