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

no-direct-mutation-state do not fail in constructor #832

Closed
TrustNik opened this issue Sep 14, 2016 · 14 comments
Closed

no-direct-mutation-state do not fail in constructor #832

TrustNik opened this issue Sep 14, 2016 · 14 comments

Comments

@TrustNik
Copy link

Hello,
I guess following code should not be considered as a problem (now it is):

export default class SomeComponent extends Component {
   constructor (props) {
           super(props);
           this.state = {a : 1};
           this.state.b = 2;
   }
...

What do you think?

@ljharb
Copy link
Member

ljharb commented Sep 14, 2016

Yes, that should warn.

@TrustNik
Copy link
Author

@ljharb could you please explain why? If I understand correctly state is initialized in the constructor, so it can't be mutated.

@ljharb
Copy link
Member

ljharb commented Sep 14, 2016

@TrustNik it doesn't cause the same problems as mutating this.state elsewhere - but mutation is never a good thing.

I can see the argument that the original spirit of this rule doesn't apply to "within the constructor", but allowing mutation isn't worth making an exception imo.

@TrustNik
Copy link
Author

@ljharb I see. Thanks for clarification.

@ivosabev
Copy link

ivosabev commented Jan 4, 2017

You should never call setState from the constructor as it brings a lot of function calls that you don't need for a component that hasn't rendered yet. It may even potentially cause double rendering.

The correct linting rule would be to remove the warning for this.state mutation and add a warning for using setState only when used inside the constructor.

@ljharb
Copy link
Member

ljharb commented Jan 4, 2017

It is correct to warn about any mutation.

@ivosabev
Copy link

ivosabev commented Jan 5, 2017

Except for setting state in the constructor. This behavior is against the official docs and some people think they should use the setState inside the constructor, when they shouldn't.

https://facebook.github.io/react/docs/state-and-lifecycle.html#do-not-modify-state-directly

@ljharb
Copy link
Member

ljharb commented Jan 5, 2017

@ivosabev Assigning to this.state once in the constructor is perfectly fine. Mutating it immediately afterwards is not.

@burabure
Copy link
Contributor

I have this rule Implemented here https://github.com/burabure/eslint-plugin-burabure/blob/master/lib/rules/no-direct-mutation-state.js @ljharb would you like this merged?

@ljharb
Copy link
Member

ljharb commented May 25, 2017

@burabure if you have a pr with tests that can fix any broken behavior, that'd be great.

burabure pushed a commit to burabure/eslint-plugin-react that referenced this issue May 25, 2017
burabure pushed a commit to burabure/eslint-plugin-react that referenced this issue May 29, 2017
burabure pushed a commit to burabure/eslint-plugin-react that referenced this issue May 30, 2017
burabure pushed a commit to burabure/eslint-plugin-react that referenced this issue May 30, 2017
@ryndshn
Copy link

ryndshn commented Apr 4, 2018

was this ever fixed? i'm still receiving a warning when modifying this.state in the constructor.

@ljharb
Copy link
Member

ljharb commented Apr 4, 2018

@ryndshn assigning to it once is fine; modifying it afterwards is intentionally disallowed.

@d07RiV
Copy link

d07RiV commented Jul 4, 2018

Why is it disallowed? I don't see any reason not to, and "never a good thing" is not a good explanation.

How do you modify state in constructor then, if it was initially assigned to in class properties?

@ljharb
Copy link
Member

ljharb commented Jul 4, 2018

You don’t. You assign your entire state all at once, in one place. If you can’t do that in a class property, you shouldn’t have a “state” class property at all - do it all in the constructor.

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

No branches or pull requests

6 participants