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

Rule suggestion: no-state-in-constructor #1810

Closed
hornta opened this issue Jun 5, 2018 · 8 comments · Fixed by #1945
Closed

Rule suggestion: no-state-in-constructor #1810

hornta opened this issue Jun 5, 2018 · 8 comments · Fixed by #1945

Comments

@hornta
Copy link

hornta commented Jun 5, 2018

INCORRECT

class MyComponent extends Component {
  constructor(props) {
    super(props);

    this.state = {
      counter: props.initialValue
    };
  }
}

CORRECT

class MyComponent extends Component {
  state = {
    counter: this.props.initialValue
  };
}

Why?
It may reduce boilerplate by not having to define a constructor just for initializing the state.

What do you think about this?

@ljharb
Copy link
Member

ljharb commented Jun 5, 2018

Since class fields are still stage 3, and not yet supported by default eslint, I'm not sure this is worth having as a rule yet - but this sounds like a great rule.

@fatfisz
Copy link
Contributor

fatfisz commented Jun 6, 2018

Just today my workmate suggested a similar idea - forbidding constructor in React components altogether.

The rationale for this is that it's easy to forget to call super with props, and according to React docs this may result in a bug:

When implementing the constructor for a React.Component subclass, you should call super(props) before any other statement. Otherwise, this.props will be undefined in the constructor, which can lead to bugs.

Then we were wondering what are the usecases for constructors, and there it is:

  • Initializing properties on this from the arguments (which includes state): since React only passes one argument to the constructor, props, which is also available as this.props when initializing through class fields, we have this covered.
  • Side effects: we don't want any in the constructor, so we ignore that.

So even with constructor we have all bases covered. As for the class fields, aren't they quite popular already in the React community? This rule wouldn't need to be turned on by default, so the decision would be up to devs. If they wanted to turn this rule on, they'd need to support class fields anyway - because otherwise where would they initialize state without a constructor?

@krawaller
Copy link

(@fatfisz) The rationale for this is that it's easy to forget to call super with props

Note that there is a rule to catch omitted super calls.

@fatfisz
Copy link
Contributor

fatfisz commented Jul 4, 2018

It doesn't enforce that you call super with the proper arguments though (not just super(), but super(props)), and that's my problem with using the constructor in React components.

@krawaller
Copy link

That's true. Issue #626 in this repo suggests such a rule, but it hasn't happened yet.

@lukyth
Copy link
Contributor

lukyth commented Aug 13, 2018

My employer happens to need this kind of rule as well. Is it okay for me to try working on this?

My idea is that the rule name is state-in-constructor. It could receive an option, always or never (default is always). If it's always, state initialization should be in a constructor. If it's never, it should be in a class property.

The reason I omitted no in the rule name is because no-state-in-constructor with an option never is a double negation and it seems confusing to me.

@ljharb
Copy link
Member

ljharb commented Aug 13, 2018

@lukyth that sounds reasonable to me! want to make the PR?

@lukyth
Copy link
Contributor

lukyth commented Aug 13, 2018

Yeah. Let me try this out. I'll open a PR once I got something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

5 participants