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

Add new rule noAccessStateInSetstate #190

Merged
merged 6 commits into from
Mar 19, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ The built-in configuration preset you get with `"extends": "tslint-react"` is se
size={size}
/>
```
- Rule options: _none_
- `jsx-ban-elements` (since v3.4.0)
- Allows blacklisting of JSX elements with an optional explanatory message in the reported failure.
- `jsx-ban-props` (since v2.3.0)
Expand Down Expand Up @@ -119,6 +120,22 @@ The built-in configuration preset you get with `"extends": "tslint-react"` is se
</button>
);
```
- Rule options: _none_
- `no-access-state-in-setstate`
- Forbids accessing component state with `this.state` within `this.setState`
calls, since React might batch multiple `this.setState` calls, thus resulting
in accessing old state. Enforces use of callback argument instead.
```ts
// bad
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.setState({
counter: this.state.counter + 1
});
// good
this.setState(
prevState => ({ counter: prevState.counter + 1 })
);
```
- Rule options: _none_

### Development

Expand Down
73 changes: 73 additions & 0 deletions src/rules/noAccessStateInSetstateRule.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/**
* @license
* Copyright 2018 Palantir Technologies, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import * as Lint from "tslint";
import { isCallExpression, isPropertyAccessExpression } from "tsutils";
import * as ts from "typescript";

export class Rule extends Lint.Rules.AbstractRule {
/* tslint:disable:object-literal-sort-keys */
public static metadata: Lint.IRuleMetadata = {
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 \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

called in batch and thus referencing old state and not the current state.`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

options: null,
optionsDescription: "",
type: "functionality",
typescriptOnly: false,
};
/* tslint:enable:object-literal-sort-keys */

public static FAILURE_STRING = "Avoid using this.state in first argument of setState.";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
return this.applyWithFunction(sourceFile, walk);
}
}

function walk(ctx: Lint.WalkContext<void>): void {
return ts.forEachChild(ctx.sourceFile, callbackForEachChild);

function callbackForEachChild(node: ts.Node): void {
if (!isCallExpression(node)) {
return ts.forEachChild(node, callbackForEachChild);
}

const isSetStateCall = node.expression.getText().startsWith("this.setState");
if (!isSetStateCall || node.arguments.length === 0) {
return ts.forEachChild(node, callbackForEachChild);
}

ts.forEachChild(node.arguments[0], callbackForEachChildInSetStateArgument);
return;
}

function callbackForEachChildInSetStateArgument(node: ts.Node): void {
if (!isPropertyAccessExpression(node)) {
return ts.forEachChild(node, callbackForEachChildInSetStateArgument);
}

if (!node.getText().startsWith("this.state.")) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

return ts.forEachChild(node, callbackForEachChildInSetStateArgument);
}

ctx.addFailureAtNode(node, Rule.FAILURE_STRING);
return;
}
}
56 changes: 56 additions & 0 deletions test/rules/no-access-state-in-setstate/test.tsx.lint
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
class SomeReactComponent extends React.Component {

someClassFunction() {

this.fooBar({
foo: this.state.foo
});

this.setState({
foo: "foo",
bar: this.barz
});

this.setState(
{
foo: "foo"
},
() => this.fooBar(this.state.foo);
);

this.setState(prevState => ({
foo: !prevState.foo
}));

this.setState((prevState, currentProps) => ({
foo: !prevState.foo,
bar: currentProps.bar
}));

this.setState({
foo: !this.state.foo
~~~~~~~~~~~~~~ [0]
});

this.setState({
foo: this.fooBar(this.state.foo)
~~~~~~~~~~~~~~ [0]
});

this.setState((prevState, currentProps) => ({
foo: !this.state.foo,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

~~~~~~~~~~~~~~ [0]
bar: currentProps.bar
}));

this.setState((prevState, currentProps) => {
this.fooBar(this.state.foo);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good idea, will do that!

Copy link
Contributor Author

@cheeZery cheeZery Jan 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

~~~~~~~~~~~~~~ [0]
return {
bar: !prevState.bar
};
});
}
}

[0]: Avoid using this.state in first argument of setState.
5 changes: 5 additions & 0 deletions test/rules/no-access-state-in-setstate/tslint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"rules": {
"no-access-state-in-setstate": true
}
}