WIP: No direct mutation state

[macovedj]

no-direct-mutation-state eslint rule

[macovedj]

This seemed like a pretty safe rule to start on. This is very much a work in progress, just thought I'd push the beginnings up in case anybody wanted to steer me away from a bad route I'm starting to take. Depending on how deep we have to check, I imagine I'll likely need some kind of recursive validator. I noticed the spreadsheet in this issue doesn't say anything about this being fixable, but it seems to me that setState or spreading could be used to fix.

[sebmck]

With #396 this can now just be:

Suggested change

	bodyBodyNode.expression.left.type === 'MemberExpression' &&
	bodyBodyNode.expression.left.object.type === 'MemberExpression' &&
	bodyBodyNode.expression.left.object.object.type === 'ThisExpression' &&
	bodyBodyNode.expression.left.object.property.value.type === 'Identifier' &&
	bodyBodyNode.expression.left.object.property.value.name === 'state'
	doesNodeMatchPattern(bodyBodyNode.expression.left, 'this.state')

Or if this check is valuable you could also do:

doesNodeMatchPattern(bodyBodyNode.expression.left, 'this.state.**')

[macovedj]

It seems like doesNodeMatchPattern was having difficulty with ThisExpressions, so I ended up having to tweak getNodeReferenceParts slightly. I imagine you may want to look over the tweak. Also I have yet to implement this pattern for what I ended up doing up in the constructor.

[macovedj]

Currently I'm thinking about the exemplified violation below:

class Hello extends React.Component {
  constructor(props) {
    super(props)

    //Assign at instance creation time, not on a callback
    doSomethingAsync(() => {
      this.state = 'bad';
    });
  }
}

My thought is that doSomethingAsync may be a function call that is defined elsewhere, so we may not have access to its body. After my first scan of reduce I think that the approach that makes sense to me is adding more info about the referenced node type in the ReferenceIdentifier nodes. This info could be grabbed from a piece of parser state that I'd make that contains function declarations when the ReferenceIdentifier nodes are created in the AST. I went ahead and implemented it, so the violation is recognized if an async function that mutates state is called in a constructor of a class component. Does this seem reasonable? I pulled the ReferenceIdentifier commit into a different PR so that it might be easier to see what's going on whilst isolated over here. #407

[macovedj]

I didn't forget about this. My employer has been very demanding lately. Hoping that will end in a weekish.

[ematipico]

I think new files should not have this comment anymore

[sebmck]

Yeah since moving to the new org new files don't need the header.

[macovedj]

I intend to pick this back up over the weekend

[stefanuros]

Hey @macovedj, I also decided this was a good first rule to start on and so I implemented it but didn't check if someone else was working on it too. Here is the commit in case you want to take a look. I noticed you didn't have test file in this pr so if you don't have one locally, you should grab the one in my commit and test with that.

[macovedj]

@stefanuros why don't you just go ahead and merge then? I prioritized some other stuff over rome for awhile, so never really finished implementing. Since you did , you should go ahead and merge and I can close this one, and I'll pick up another one when I can.

[stefanuros]

@macovedj I made a PR, #582. However, I seem to have problems with the scripts/dev-rome lint command so I cannot proceed until that is fixed.

[macovedj]

@stefanuros I can see you're actively working on your lint issue. If you get stuck and want a second pair of eyes, I'm happy to lend them. I haven't looked at your PR in depth.