Incorrect no-direct-mutation-state Warning

[BrodaNoel]

https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-direct-mutation-state.md

This is happening right now:

constructor(props) {
    super(props);

    // No warning here
    this.state.randomId = Math.round(Math.random() * 1000000);

    this.props.tables.forEach((table, i) => {
      if (table.isOpen) {
        // Here we have a warning
        this.state[`table${i}Shown`] = true;
      }
    });
  }

[ljharb]

The first one should be throwing at runtime, since this.state is undefined

[BrodaNoel]

What if I have:

state = {};

constructor(props) {
    super(props);

    // No warning here
    this.state.randomId = Math.round(Math.random() * 1000000);

    this.props.tables.forEach((table, i) => {
      if (table.isOpen) {
        // Here we have a warning
        this.state[`table${i}Shown`] = true;
      }
    });
  }

[ljharb]

in that case, I would certainly expect a warning there.

[BrodaNoel]

Actually there should not be warnings in any of both places

[BrodaNoel]

The "direct state mutation" is able in any part of the constructor (sync)

[ljharb]

The rule only allows assigning this.state in the constructor. It is never OK to mutate it.

[BrodaNoel]

Ok, so, clearly there is something that I'm missing. Could you explain to me why I see a warning inside the forEach? Thanks

…

El 7 jun 2018, a las 02:44, Jordan Harband ***@***.***> escribió: The rule only allows assigning this.state in the constructor. It is never OK to mutate it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[ljharb]

Sure - first, the state = {} in the class body is equivalent to putting this.state = {} in the constructor just after the super call. The bug is that the warnings aren’t the same in those two cases.

Second, the rule is designed to prevent any mutation of the state object after it’s created. So, you should see a warning inside the forEach, but also on the randomId line.

In other words, you should build up the entire state object before assigning it to the instance.

[BrodaNoel]

Actually, I'm not seen and I should not see a warning in the randomId line.
The state can be manipulated in the constructor as much as we want. It' doesn't affect the React state.

I got your point after checking what you said in #832

I disagree about warning in any place of the constructor. The react-state doesn't know anything about this.state var yet. It's just a var manipulation, not a "React State manipulation". But, anyways... Bug reported.

Thank you a lot for your time.

[ljharb]

The point of the rule is to forbid mutating state in the constructor ever. It does, also, have an effect - mutating an object after creation is slower than creating it all at once.

[ljharb]

Additionally, mutation of any kind is always a code smell, especially in react. If you want a different object, clone it and assign the new one.