From 0caf7dfc6db9954e994bfc074a037fd36c132b2b Mon Sep 17 00:00:00 2001 From: Carsten Dietzel Date: Sat, 8 Dec 2018 21:46:15 +0100 Subject: [PATCH 1/6] Add new rule noAccessStateInSetstate Inspired by ESLint's no-access-state-in-setstate rule --- README.md | 17 +++++ src/rules/noAccessStateInSetstateRule.ts | 69 +++++++++++++++++++ .../no-access-state-in-setstate/test.tsx.lint | 68 ++++++++++++++++++ .../no-access-state-in-setstate/tslint.json | 5 ++ 4 files changed, 159 insertions(+) create mode 100644 src/rules/noAccessStateInSetstateRule.ts create mode 100644 test/rules/no-access-state-in-setstate/test.tsx.lint create mode 100644 test/rules/no-access-state-in-setstate/tslint.json diff --git a/README.md b/README.md index c7ab243..ab0f25b 100644 --- a/README.md +++ b/README.md @@ -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) @@ -119,6 +120,22 @@ The built-in configuration preset you get with `"extends": "tslint-react"` is se ); ``` + - 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 + this.setState({ + counter: this.state.counter + 1 + }); + // good + this.setState( + prevState => ({ counter: prevState.counter + 1 }) + ); + ``` + - Rule options: _none_ ### Development diff --git a/src/rules/noAccessStateInSetstateRule.ts b/src/rules/noAccessStateInSetstateRule.ts new file mode 100644 index 0000000..c7c7f5f --- /dev/null +++ b/src/rules/noAccessStateInSetstateRule.ts @@ -0,0 +1,69 @@ +/** + * @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 } 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 \ + called in batch and thus referencing old state and not the current state.`, + options: null, + optionsDescription: "", + type: "functionality", + typescriptOnly: false, + }; + /* tslint:enable:object-literal-sort-keys */ + + public static FAILURE_STRING = "Use callback in setState when referencing the previous state."; + + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + return this.applyWithFunction(sourceFile, walk); + } +} + +function walk(ctx: Lint.WalkContext): void { + return ts.forEachChild(ctx.sourceFile, cb); + + function cb(node: ts.Node): void { + if (!isCallExpression(node)) { + return ts.forEachChild(node, cb); + } + if (isStateUsedInSetStateWith(node)) { + ctx.addFailureAtNode(node, Rule.FAILURE_STRING); + } + return; + } +} + +function isStateUsedInSetStateWith(callExpression: ts.CallExpression): boolean { + if (callExpression.expression.getText() !== "this.setState") { + return false; + } + if (callExpression.arguments.length === 0) { + return false; + } + + const firstCallExpressionArgument = callExpression.arguments[0]; + const argumentAsText = firstCallExpressionArgument.getText(); + return argumentAsText.indexOf("this.state.") !== -1; +} diff --git a/test/rules/no-access-state-in-setstate/test.tsx.lint b/test/rules/no-access-state-in-setstate/test.tsx.lint new file mode 100644 index 0000000..86ce666 --- /dev/null +++ b/test/rules/no-access-state-in-setstate/test.tsx.lint @@ -0,0 +1,68 @@ +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, +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + bar: currentProps.bar +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + })); +~~~~~~~~~~~ [0] + + this.setState((prevState, currentProps) => { + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + this.fooBar(this.state.foo); +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + return { +~~~~~~~~~~~~~~~~~~~~ + bar: !prevState.bar +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + }; +~~~~~~~~~~~~~~ + }); +~~~~~~~~~~ [0] + } +} + +[0]: Use callback in setState when referencing the previous state. diff --git a/test/rules/no-access-state-in-setstate/tslint.json b/test/rules/no-access-state-in-setstate/tslint.json new file mode 100644 index 0000000..666f783 --- /dev/null +++ b/test/rules/no-access-state-in-setstate/tslint.json @@ -0,0 +1,5 @@ +{ + "rules": { + "no-access-state-in-setstate": true + } +} From 8ddc00ff9a69eb33051d3cf125efe8754e05e1c7 Mon Sep 17 00:00:00 2001 From: Carsten Dietzel Date: Thu, 17 Jan 2019 23:12:28 +0100 Subject: [PATCH 2/6] Improve determining error-prone access to this.state --- src/rules/noAccessStateInSetstateRule.ts | 40 ++++++++++--------- .../no-access-state-in-setstate/test.tsx.lint | 22 +++------- 2 files changed, 27 insertions(+), 35 deletions(-) diff --git a/src/rules/noAccessStateInSetstateRule.ts b/src/rules/noAccessStateInSetstateRule.ts index c7c7f5f..309e430 100644 --- a/src/rules/noAccessStateInSetstateRule.ts +++ b/src/rules/noAccessStateInSetstateRule.ts @@ -16,7 +16,7 @@ */ import * as Lint from "tslint"; -import { isCallExpression } from "tsutils"; +import { isCallExpression, isPropertyAccessExpression } from "tsutils"; import * as ts from "typescript"; export class Rule extends Lint.Rules.AbstractRule { @@ -34,7 +34,7 @@ export class Rule extends Lint.Rules.AbstractRule { }; /* tslint:enable:object-literal-sort-keys */ - public static FAILURE_STRING = "Use callback in setState when referencing the previous state."; + public static FAILURE_STRING = "Avoid using this.state in first argument of setState."; public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { return this.applyWithFunction(sourceFile, walk); @@ -42,28 +42,32 @@ export class Rule extends Lint.Rules.AbstractRule { } function walk(ctx: Lint.WalkContext): void { - return ts.forEachChild(ctx.sourceFile, cb); + return ts.forEachChild(ctx.sourceFile, callbackForEachChild); - function cb(node: ts.Node): void { + function callbackForEachChild(node: ts.Node): void { if (!isCallExpression(node)) { - return ts.forEachChild(node, cb); + return ts.forEachChild(node, callbackForEachChild); } - if (isStateUsedInSetStateWith(node)) { - ctx.addFailureAtNode(node, Rule.FAILURE_STRING); + + 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 isStateUsedInSetStateWith(callExpression: ts.CallExpression): boolean { - if (callExpression.expression.getText() !== "this.setState") { - return false; - } - if (callExpression.arguments.length === 0) { - return false; - } + function callbackForEachChildInSetStateArgument(node: ts.Node): void { + if (!isPropertyAccessExpression(node)) { + return ts.forEachChild(node, callbackForEachChildInSetStateArgument); + } + + if (!node.getText().startsWith("this.state.")) { + return ts.forEachChild(node, callbackForEachChildInSetStateArgument); + } - const firstCallExpressionArgument = callExpression.arguments[0]; - const argumentAsText = firstCallExpressionArgument.getText(); - return argumentAsText.indexOf("this.state.") !== -1; + ctx.addFailureAtNode(node, Rule.FAILURE_STRING); + return; + } } diff --git a/test/rules/no-access-state-in-setstate/test.tsx.lint b/test/rules/no-access-state-in-setstate/test.tsx.lint index 86ce666..e40e608 100644 --- a/test/rules/no-access-state-in-setstate/test.tsx.lint +++ b/test/rules/no-access-state-in-setstate/test.tsx.lint @@ -28,41 +28,29 @@ class SomeReactComponent extends React.Component { })); this.setState({ - ~~~~~~~~~~~~~~~ foo: !this.state.foo -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + ~~~~~~~~~~~~~~ [0] }); -~~~~~~~~~~ [0] this.setState({ - ~~~~~~~~~~~~~~~ foo: this.fooBar(this.state.foo) -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + ~~~~~~~~~~~~~~ [0] }); -~~~~~~~~~~ [0] this.setState((prevState, currentProps) => ({ - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ foo: !this.state.foo, -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + ~~~~~~~~~~~~~~ [0] bar: currentProps.bar -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ })); -~~~~~~~~~~~ [0] this.setState((prevState, currentProps) => { - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ this.fooBar(this.state.foo); -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + ~~~~~~~~~~~~~~ [0] return { -~~~~~~~~~~~~~~~~~~~~ bar: !prevState.bar -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ }; -~~~~~~~~~~~~~~ }); -~~~~~~~~~~ [0] } } -[0]: Use callback in setState when referencing the previous state. +[0]: Avoid using this.state in first argument of setState. From 5e19d469ea373a0bbec5c31cb8f8fa1bb4127989 Mon Sep 17 00:00:00 2001 From: Carsten Dietzel Date: Wed, 13 Feb 2019 08:29:12 +0100 Subject: [PATCH 3/6] Improve performance by only linting classes and preventing usage of getText --- src/rules/noAccessStateInSetstateRule.ts | 37 ++++++++++++++----- .../no-access-state-in-setstate/test.tsx.lint | 8 ++++ 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/src/rules/noAccessStateInSetstateRule.ts b/src/rules/noAccessStateInSetstateRule.ts index 309e430..dd52cbd 100644 --- a/src/rules/noAccessStateInSetstateRule.ts +++ b/src/rules/noAccessStateInSetstateRule.ts @@ -16,7 +16,7 @@ */ import * as Lint from "tslint"; -import { isCallExpression, isPropertyAccessExpression } from "tsutils"; +import { isCallExpression, isClassDeclaration, isPropertyAccessExpression } from "tsutils"; import * as ts from "typescript"; export class Rule extends Lint.Rules.AbstractRule { @@ -45,29 +45,48 @@ function walk(ctx: Lint.WalkContext): 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, callbackForEachChild); + return ts.forEachChild(node, callbackForEachChildInClass); } - const isSetStateCall = node.expression.getText().startsWith("this.setState"); - if (!isSetStateCall || node.arguments.length === 0) { - return ts.forEachChild(node, callbackForEachChild); + 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); - return; } function callbackForEachChildInSetStateArgument(node: ts.Node): void { - if (!isPropertyAccessExpression(node)) { + if (!isPropertyAccessExpression(node) || !isPropertyAccessExpression(node.expression)) { return ts.forEachChild(node, callbackForEachChildInSetStateArgument); } - if (!node.getText().startsWith("this.state.")) { + if ( + node.expression.expression.kind !== ts.SyntaxKind.ThisKeyword || + node.expression.name.text !== "state" + ) { return ts.forEachChild(node, callbackForEachChildInSetStateArgument); } ctx.addFailureAtNode(node, Rule.FAILURE_STRING); - return; } } diff --git a/test/rules/no-access-state-in-setstate/test.tsx.lint b/test/rules/no-access-state-in-setstate/test.tsx.lint index e40e608..dbd521f 100644 --- a/test/rules/no-access-state-in-setstate/test.tsx.lint +++ b/test/rules/no-access-state-in-setstate/test.tsx.lint @@ -27,6 +27,14 @@ class SomeReactComponent extends React.Component { bar: currentProps.bar })); + this.setState({ + foo: window.history.length + }); + + this.setState({ + foo: !this.props.bar + }); + this.setState({ foo: !this.state.foo ~~~~~~~~~~~~~~ [0] From a81a966be38568707fbd1d00b248f8410aa29acc Mon Sep 17 00:00:00 2001 From: Carsten Dietzel Date: Mon, 18 Mar 2019 21:49:08 +0100 Subject: [PATCH 4/6] Fix code block indention in README --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index ab0f25b..d331ee6 100644 --- a/README.md +++ b/README.md @@ -125,7 +125,7 @@ The built-in configuration preset you get with `"extends": "tslint-react"` is se - 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 + ```ts // bad this.setState({ counter: this.state.counter + 1 From 0ce297b22f4234da91441eb9bc7841b9f0943957 Mon Sep 17 00:00:00 2001 From: Carsten Dietzel Date: Mon, 18 Mar 2019 22:19:09 +0100 Subject: [PATCH 5/6] Fix dedent usage --- src/rules/noAccessStateInSetstateRule.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/rules/noAccessStateInSetstateRule.ts b/src/rules/noAccessStateInSetstateRule.ts index dd52cbd..afc19b9 100644 --- a/src/rules/noAccessStateInSetstateRule.ts +++ b/src/rules/noAccessStateInSetstateRule.ts @@ -24,9 +24,11 @@ export class Rule extends Lint.Rules.AbstractRule { 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 \ - called in batch and thus referencing old state and not the current state.`, + 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. + See [setState()](https://reactjs.org/docs/react-component.html#setstate) in the React API reference. + `, options: null, optionsDescription: "", type: "functionality", From e2b08c5af77f1553e02ec1f0ae03eb1fbec20316 Mon Sep 17 00:00:00 2001 From: Carsten Dietzel Date: Mon, 18 Mar 2019 22:20:04 +0100 Subject: [PATCH 6/6] Provide more specific failure strings by differentiating arg type --- src/rules/noAccessStateInSetstateRule.ts | 37 ++++++++++++++++--- .../no-access-state-in-setstate/test.tsx.lint | 7 ++-- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/src/rules/noAccessStateInSetstateRule.ts b/src/rules/noAccessStateInSetstateRule.ts index afc19b9..8f197f7 100644 --- a/src/rules/noAccessStateInSetstateRule.ts +++ b/src/rules/noAccessStateInSetstateRule.ts @@ -36,7 +36,11 @@ export class Rule extends Lint.Rules.AbstractRule { }; /* tslint:enable:object-literal-sort-keys */ - public static FAILURE_STRING = "Avoid using this.state in first argument of setState."; + public static OBJECT_ARG_FAILURE = + "References to this.state are not allowed in the setState state change object."; + + public static CALLBACK_ARG_FAILURE = + "References to this.state are not allowed in the setState updater, use the callback arguments instead."; public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { return this.applyWithFunction(sourceFile, walk); @@ -74,21 +78,42 @@ function walk(ctx: Lint.WalkContext): void { return; } - ts.forEachChild(node.arguments[0], callbackForEachChildInSetStateArgument); + const firstArgument = node.arguments[0]; + + if (ts.isObjectLiteralExpression(firstArgument)) { + ts.forEachChild(firstArgument, callbackForEachChildInSetStateObjectArgument); + } else if (ts.isArrowFunction(firstArgument) || ts.isFunctionExpression(firstArgument)) { + ts.forEachChild(firstArgument, callbackForEachChildInSetStateCallbackArgument); + } + } + + function callbackForEachChildInSetStateObjectArgument(node: ts.Node): void { + if (!isPropertyAccessExpression(node) || !isPropertyAccessExpression(node.expression)) { + return ts.forEachChild(node, callbackForEachChildInSetStateObjectArgument); + } + + if ( + node.expression.expression.kind !== ts.SyntaxKind.ThisKeyword || + node.expression.name.text !== "state" + ) { + return ts.forEachChild(node, callbackForEachChildInSetStateObjectArgument); + } + + ctx.addFailureAtNode(node, Rule.OBJECT_ARG_FAILURE); } - function callbackForEachChildInSetStateArgument(node: ts.Node): void { + function callbackForEachChildInSetStateCallbackArgument(node: ts.Node): void { if (!isPropertyAccessExpression(node) || !isPropertyAccessExpression(node.expression)) { - return ts.forEachChild(node, callbackForEachChildInSetStateArgument); + return ts.forEachChild(node, callbackForEachChildInSetStateCallbackArgument); } if ( node.expression.expression.kind !== ts.SyntaxKind.ThisKeyword || node.expression.name.text !== "state" ) { - return ts.forEachChild(node, callbackForEachChildInSetStateArgument); + return ts.forEachChild(node, callbackForEachChildInSetStateCallbackArgument); } - ctx.addFailureAtNode(node, Rule.FAILURE_STRING); + ctx.addFailureAtNode(node, Rule.CALLBACK_ARG_FAILURE); } } diff --git a/test/rules/no-access-state-in-setstate/test.tsx.lint b/test/rules/no-access-state-in-setstate/test.tsx.lint index dbd521f..008ecf8 100644 --- a/test/rules/no-access-state-in-setstate/test.tsx.lint +++ b/test/rules/no-access-state-in-setstate/test.tsx.lint @@ -47,13 +47,13 @@ class SomeReactComponent extends React.Component { this.setState((prevState, currentProps) => ({ foo: !this.state.foo, - ~~~~~~~~~~~~~~ [0] + ~~~~~~~~~~~~~~ [1] bar: currentProps.bar })); this.setState((prevState, currentProps) => { this.fooBar(this.state.foo); - ~~~~~~~~~~~~~~ [0] + ~~~~~~~~~~~~~~ [1] return { bar: !prevState.bar }; @@ -61,4 +61,5 @@ class SomeReactComponent extends React.Component { } } -[0]: Avoid using this.state in first argument of setState. +[0]: References to this.state are not allowed in the setState state change object. +[1]: References to this.state are not allowed in the setState updater, use the callback arguments instead.