jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
8.97k stars 2.77k forks source link

Rule proposal: check constructor params for props and context #626

Open ide opened 8 years ago

ide commented 8 years ago

This rule would check for:

class X extends Component {
  constructor(props, context) {
    super(props, context);
  }
}

React is lenient, so constructor() { super(); } works, so this is just a stylistic preference.

ljharb commented 8 years ago

It should work with ...args also.

OliverJAsh commented 6 years ago

React is lenient, so constructor() { super(); } works, so this is just a stylistic preference. šŸ‘ 3

I would argue this is more than a stylistic preferenceā€”it can actually lead to bugs. Consider this example, where a method that relies on props is invoked from the constructor:

class Foo extends Component {
  constructor()
    super();
    this.state = this.getInitialState();
  }
  getInitialState() {
    // ERROR: `props` is `undefined` because the super constructor did not receive `props`
    const x = this.props.y + 1;
    return { x }
  }
}
ljharb commented 6 years ago

@OliverJAsh in your example, super() will set this.props for you - React.Component does not use any of the constructor arguments, to handle this case.

OliverJAsh commented 6 years ago

@ljharb

If you want to use this.props in the constructor, you need to pass props to super. Otherwise, it doesn't matter because React sets .props on the instance from the outside immediately after calling the constructor.

https://discuss.reactjs.org/t/should-we-include-the-props-parameter-to-class-constructors-when-declaring-components-using-es6-classes/2781/2

In my example, I'm calling a method (which relies on this.props) from the constructor. So, it will be undefined.

ljharb commented 6 years ago

interesting, fair enough.

ljharb commented 6 years ago

In that case, the rule should probably enforce props, context or ...args

joetidee commented 6 years ago

I would also like to see this rule in place, i.e. if props is used in the constructor then constructor(props) and super(props) must exist. An example of where this might cause problems:

class Foo extends Component {
    constructor() {
        super();
        this.someObj = {
            bar: this.props.intl.formatMessage(...),
        };
    }
}

Eslint will not currently thrown any errors here. I would suggest enforcing no use of this.props in the constructor and instead recommend just using props.