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 3 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
92 changes: 92 additions & 0 deletions src/rules/noAccessStateInSetstateRule.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/**
* @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, isClassDeclaration, 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 (!isClassDeclaration(node)) {
return;
}

ts.forEachChild(node, callbackForEachChildInClass);
}

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

const callExpressionArguments = node.arguments;

if (!isPropertyAccessExpression(node.expression) || callExpressionArguments.length === 0) {
return;
}

const propertyAccessExpression = node.expression;

const isThisPropertyAccess = propertyAccessExpression.expression.kind === ts.SyntaxKind.ThisKeyword;
const isSetStateCall = propertyAccessExpression.name.text === "setState";

if (!isThisPropertyAccess || !isSetStateCall) {
return;
}

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

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

if (
node.expression.expression.kind !== ts.SyntaxKind.ThisKeyword ||
node.expression.name.text !== "state"
) {
return ts.forEachChild(node, callbackForEachChildInSetStateArgument);
}

ctx.addFailureAtNode(node, Rule.FAILURE_STRING);
}
}
64 changes: 64 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,64 @@
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: window.history.length
});

this.setState({
foo: !this.props.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
}
}