Add new rule noAccessStateInSetstate #190

cheeZery · 2018-12-08T21:12:19Z

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!

Inspired by ESLint's no-access-state-in-setstate rule

karfau

I fully agree that it is a great first step forward. Would like to see it being available.

karfau · 2019-01-16T07:33:35Z

test/rules/no-access-state-in-setstate/test.tsx.lint

+
+        this.setState((prevState, currentProps) => {
+        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+            this.fooBar(this.state.foo);


For this case the error message might read a bit strange, since the code is using the callback.
Maybe Use first argument from callback inside setState instead of this.state.?

Very good idea, will do that!

Okay, unfortunately it might be a little difficult. In my now updated code, you'd have to copy the callbackForEachChildInSetStateArgument function, for checking a callback and adding another failure text to the node. At least that's the only way I can think of right now, but I don't have that much experience with TSLint yet :-)

So for a quick solution I simply made the failure text a bit more generic.

karfau · 2019-01-16T11:06:43Z

test/rules/no-access-state-in-setstate/test.tsx.lint

+
+        this.setState((prevState, currentProps) => ({
+        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+            foo: !this.state.foo,


it would of course be much nicer to have the marker only on the occurrence(s) of this.state but I would consider it nice to have.

I sat down again with my code and now I'm pretty sure I found a still very easy and even more reliable way to not only find accesses of this.state but also can now display the error message exactly at the right node.
I will push the changes at the weekend! Thanks for the idea!

Or even better push it right away. Looking forward to your review!
I now first check if a node is a setState call and if so, I check each child of the first argument of that call, determining if that node is a this.state access.

karfau

Wow, really nice!

karfau · 2019-01-18T08:06:15Z

src/rules/noAccessStateInSetstateRule.ts

+            return ts.forEachChild(node, callbackForEachChildInSetStateArgument);
+        }
+
+        if (!node.getText().startsWith("this.state.")) {


Since this.state could also be a umber, boolean or be to some function without accessing it's fields I would suggest to omit the last . in the search expression. (You might want to add a test case for that one.)

I just realize that I maybe should start the linter with checking if we're even in some kind of React component class.
And yeah, I guess you're right, this.state doesn't necessarly need to be an object. I tried to find some source telling you that you shouldn't store some primitive value directly as a state, but couldn't find any, so maybe that isn't even a code smell.. There will be a warning from React though (this one).
But I will transform that check to a RegEx, the one in Line 52 as well :-) Something like ^this\.state\b

Yes, there might be some things that can be done regarding performance I guess.
And I would of course prefer a fast rule over a slow one, but a slow one would even be better then none.
So it's up to you and since no contributor has reacted on this PR yet you might as well improve inside this PR.

Yeah, we should aim for a performant, fast rule, so I'll gladly check with TSLint's guide again. Anything particular you have in mind?

I think the current impllementation makes heavy use of the getText method, and the guide has some comments regarding it. I think this and checking that the file even can contain/contains this.setState might be the most low hanging fruits.

Sorry, it took me some time. But now I guess I did improve the performance. I got rid of both getText calls, and in addition only walk the children of classes.

This linter will still run in every class, React.Component or not, but as far as I know you'd need a TypedRule to prevent this. But definitely something that could still be improved.

Thanks for the helpful suggestions, sorry for the long delay, and again looking forward to the code review!

…etText

karfau

This is awesome, looking forward to use it.
(Not sure what it means that I approve this, but doing it anyways, since I would love to see this being merged.)

karfau · 2019-03-01T06:42:36Z

@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.

adidahiya · 2019-03-12T22:32:15Z

README.md

+  calls, since React might batch multiple `this.setState` calls, thus resulting
+  in accessing old state. Enforces use of callback argument instead.
+    ```ts
+  // bad


this did not get syntax highlighted / formatted correctly -- check out the markdown preview

adidahiya · 2019-03-12T22:33:10Z

src/rules/noAccessStateInSetstateRule.ts

+        ruleName: "no-access-state-in-setstate",
+        description: "Reports usage of this.state within setState",
+        rationale: Lint.Utils.dedent
+            `Usage of this.state might result in errors when two state calls are \


you don't need the trailing \, dedent handles that.

also the leading backtick should go right after dedent, and the trailing backtick should go after the text block on a new line. see https://github.com/palantir/tslint/blob/e080fc4ceb58e907d48bf5419ceb96e6899dd5d5/src/rules/strictBooleanExpressionsRule.ts#L37, for example

adidahiya · 2019-03-12T22:39:27Z

src/rules/noAccessStateInSetstateRule.ts

+        description: "Reports usage of this.state within setState",
+        rationale: Lint.Utils.dedent
+            `Usage of this.state might result in errors when two state calls are \
+            called in batch and thus referencing old state and not the current state.`,


It would be good to reference these react docs in this rationale. you can use markdown

Added a link to the docs. Is the phrasing alright? (English being not my first language and all)

adidahiya · 2019-03-12T22:42:20Z

src/rules/noAccessStateInSetstateRule.ts

+    };
+    /* tslint:enable:object-literal-sort-keys */
+
+    public static FAILURE_STRING = "Avoid using this.state in first argument of setState.";


how about we say this instead:

for uses of this.state in the callback update syntax: "References to this.state are not allowed in the setState updater, use the callback arguments instead."

for uses of this.state in the stateChange object syntax: "References to this.state are not allowed in the setState state change object."

Good idea! Unfortunately the only way I came up to achieve this was to duplicate the callback function which checks the setState argument. Any better idea? :-)

I have some ideas for cleaning this up a bit, I can push an additional commit after this merges

Maybe you can share your ideas now, since I will refactor this again a bit. Or you just push them, as soon as I provide you guys with the new version!

cheeZery · 2019-03-18T09:23:49Z

@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 · 2019-03-19T02:01:21Z

thanks @cheeZery!

adidahiya · 2019-03-19T02:10:13Z

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.

This reverts commit 987a95e.

karfau · 2019-03-19T07:56:58Z

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 · 2019-03-19T15:18:14Z

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 · 2019-03-19T18:22:31Z

@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 - #190 (comment). 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 · 2019-03-19T18:46:37Z

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

cheeZery · 2019-03-30T18:49:04Z

@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! :-)

Add new rule noAccessStateInSetstate

0caf7df

Inspired by ESLint's no-access-state-in-setstate rule

cheeZery mentioned this pull request Jan 16, 2019

New rule suggestion: no-access-state-in-setstate #172

Closed

karfau reviewed Jan 16, 2019

View reviewed changes

Improve determining error-prone access to this.state

8ddc00f

karfau reviewed Jan 18, 2019

View reviewed changes

Improve performance by only linting classes and preventing usage of g…

5e19d46

…etText

karfau approved these changes Feb 14, 2019

View reviewed changes

adidahiya suggested changes Mar 12, 2019

View reviewed changes

cheeZery added 3 commits March 18, 2019 21:49

Fix code block indention in README

a81a966

Fix dedent usage

0ce297b

Provide more specific failure strings by differentiating arg type

e2b08c5

adidahiya approved these changes Mar 19, 2019

View reviewed changes

adidahiya merged commit 987a95e into palantir:master Mar 19, 2019

adidahiya added a commit that referenced this pull request Mar 19, 2019

Revert "New rule no-access-state-in-setstate (#190)"

ed56e53

This reverts commit 987a95e.

adidahiya mentioned this pull request Mar 19, 2019

Revert "Add new rule noAccessStateInSetstate" #211

Merged

adidahiya added a commit that referenced this pull request Mar 19, 2019

Revert "New rule no-access-state-in-setstate (#190)"

691dc3b

This reverts commit 987a95e.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new rule noAccessStateInSetstate #190

Add new rule noAccessStateInSetstate #190

cheeZery commented Dec 8, 2018 •

edited

Loading

karfau left a comment

karfau Jan 16, 2019

cheeZery Jan 17, 2019

cheeZery Jan 17, 2019 •

edited

Loading

karfau Jan 16, 2019

cheeZery Jan 17, 2019

cheeZery Jan 17, 2019

karfau left a comment

karfau Jan 18, 2019

cheeZery Jan 18, 2019

karfau Jan 18, 2019

cheeZery Jan 21, 2019

karfau Jan 21, 2019

cheeZery Feb 13, 2019

karfau left a comment

karfau commented Mar 1, 2019

adidahiya Mar 12, 2019

adidahiya Mar 12, 2019

adidahiya Mar 12, 2019

cheeZery Mar 18, 2019

adidahiya Mar 12, 2019

cheeZery Mar 18, 2019

adidahiya Mar 19, 2019

cheeZery Mar 19, 2019

cheeZery commented Mar 18, 2019 •

edited

Loading

adidahiya commented Mar 19, 2019

adidahiya commented Mar 19, 2019

karfau commented Mar 19, 2019

adidahiya commented Mar 19, 2019

cheeZery commented Mar 19, 2019

adidahiya commented Mar 19, 2019

cheeZery commented Mar 30, 2019

Add new rule noAccessStateInSetstate #190

Add new rule noAccessStateInSetstate #190

Conversation

cheeZery commented Dec 8, 2018 • edited Loading

Overview of change:

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

karfau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheeZery Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karfau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karfau left a comment

Choose a reason for hiding this comment

karfau commented Mar 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheeZery commented Mar 18, 2019 • edited Loading

adidahiya commented Mar 19, 2019

adidahiya commented Mar 19, 2019

karfau commented Mar 19, 2019

adidahiya commented Mar 19, 2019

cheeZery commented Mar 19, 2019

adidahiya commented Mar 19, 2019

cheeZery commented Mar 30, 2019

cheeZery commented Dec 8, 2018 •

edited

Loading

cheeZery Jan 17, 2019 •

edited

Loading

cheeZery commented Mar 18, 2019 •

edited

Loading