Add new rule noAccessStateInSetstate

[cheeZery]

Fixes #172 (and #80 to some extent)

Overview of change:

Took ESLint-plugin-React's no-access-state-in-setstate rule and implemented it in tslint-react. I know no maintainer seems to have reviewed #172 yet, but I took a swing at it anyway, hope that was okay!

Is there anything you'd like reviewers to focus on?

You will find that I did the checks with pretty simple string comparisons (e.g. expression.getText().indexOf("this.state.") !== -1;). I started creating the checks with actually using the AST but found that the string solution is not only imho more reliable but of course also a lot easier to write and understand. But maybe there're better ways to check it!

No matter the way the check of the this.setState argument is implemented, there are still ways to bypass the check and creating - if you want so - false-positives.

class MyReactComponent extend React.Component {
  // ...

  myFunction() {
    this.setState({
      foo: this.state.foo + 1 // Rule will detect this
    });

    const foo = this.state.foo;
    this.setState({
      foo: foo + 1 // Still using this.state, rule won't mark it
    });

    this.setState({
      foo: getFoo() // Also this.state, won't be detected
    })
  }

  getFoo() {
    return this.state.foo;
  }
}

You'd have to determine every variable containing (or deriving from) the current state and every functions whose return value depends on it, and then check if those variables and methods are used in this.setState. ESLint-plugin-React even does something like that. But I guess my pull request atleast offers a good starting point and will catch most of the error-prone code lines. Since English isn't my first language, all texts (e.g. rule description) might be improvable :-) Looking forward to your review!

[karfau]

@cheeZery I would really love to have this rule availble. Since the last PR in this repo has been merged in July and tslint is on it's way out my hope for any changes in this repo are not so big. What do you think about integrating your rule to this rule set instead: https://github.com/bettermarks/bm-tslint-rules ? (which my colleagues and I are currently maintaining) There are many options on how to achieve this,

• the easiest for us would be to add the code ad test for your rule to that repo (feel free to file a PR, but I can also do that)
• you could of course also find any other way to provide your rule via npm (e.g. just putting them in an own gist or repo could also be an option) Please let me know your thoughts about this.

[cheeZery]

@karfau sorry for the delay again. And thanks for the review. Normally I would gladly suggest to suggest add the code to another TSLint plugin repo. But now since @adidahiya reviewed my changes, this might get merged anyway :-)

@adidahiya thanks for your review! I will get to the fixes soon! Although I'm sure this rule would just be added since there already was an open PR. Otherwise we probably would wait a few weeks/months until we maybe just can use https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-access-state-in-setstate.md

[adidahiya]

thanks @cheeZery!

[adidahiya]

Actually I merged this too soon. This rule should use type information and only cover React components. Sorry, I will have to revert it for now, you are welcome to open a new PR with that change.

[karfau]

Wow, this way of handling the issue is really kind of a harsh way to handle it in my opinion. Making the rule available but not enabling it by default (which I would assume is the case anyways, but I didn't check) so people can file improvement PRs would make much more sense for me.

And it is already only checking classes, which should be a very (beside class components) in any react project...

just my 2 cents

[adidahiya]

It's also pretty easy to write this as a custom rule in your project or your own rule set, try it out in a sizable React project, and merge it into the "official" tslint-react repo once it's well-tested.

And it is already only checking classes, which should be a very (beside class components) in any react project...

I don't think this is a good assumption; I have worked on many React projects where this is not the case.

[cheeZery]

@adidahiya you are definitely right in that this should be a typed rule and only run in React.Component classes. We even discussed this earlier in this PR - https://github.com/palantir/tslint-react/pull/190#discussion_r256274600. Back then I thought the TypedRule overhead would be a bigger performance impact than just lint every class. But you are probably right, so I will refactor this to only run in React.Component (and React.PureComponent since they can have a state too). Thanks for your thorough review.

[adidahiya]

Yeah, I noticed that comment a little too late unfortunately. Thanks for understanding.

[cheeZery]

@adidahiya I might need some help or a tip for this one, since this is my first time working with the type checker 😳 and it would also be the first rule using type information in the repo.

So first of all I put a tsconfig file in the test dir to get a program and with that the type checker. From there I tried some stuff but couldn't really came up with something useful. In the new TypedRule I checked if the class declaration had heritage clauses, and if so, I planed on getTypeAtLocation of those clauses. And from there I would probably check, if the type's base is a Component from the react module. Does that make sense? And how would you achieve this? Unfortunately I wasn't able to find a TSLint rule (not in the core, nor in any plugin I've looked up), which checks for a specific parent in a heritage clause.

Any help is appreciated! :-)