From 987a95e87497f250e6d7f6dbaeb8abd12fa116cf Mon Sep 17 00:00:00 2001 From: Carsten Dietzel Date: Tue, 19 Mar 2019 03:01:41 +0100 Subject: [PATCH] New rule no-access-state-in-setstate (#190) --- README.md | 17 +++ src/rules/noAccessStateInSetstateRule.ts | 119 ++++++++++++++++++ .../no-access-state-in-setstate/test.tsx.lint | 65 ++++++++++ .../no-access-state-in-setstate/tslint.json | 5 + 4 files changed, 206 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 7f13511..b7297a4 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) @@ -121,6 +122,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..8f197f7 --- /dev/null +++ b/src/rules/noAccessStateInSetstateRule.ts @@ -0,0 +1,119 @@ +/** + * @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 + 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", + typescriptOnly: false, + }; + /* tslint:enable:object-literal-sort-keys */ + + 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); + } +} + +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, 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; + } + + 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 callbackForEachChildInSetStateCallbackArgument(node: ts.Node): void { + if (!isPropertyAccessExpression(node) || !isPropertyAccessExpression(node.expression)) { + return ts.forEachChild(node, callbackForEachChildInSetStateCallbackArgument); + } + + if ( + node.expression.expression.kind !== ts.SyntaxKind.ThisKeyword || + node.expression.name.text !== "state" + ) { + return ts.forEachChild(node, callbackForEachChildInSetStateCallbackArgument); + } + + 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 new file mode 100644 index 0000000..008ecf8 --- /dev/null +++ b/test/rules/no-access-state-in-setstate/test.tsx.lint @@ -0,0 +1,65 @@ +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, + ~~~~~~~~~~~~~~ [1] + bar: currentProps.bar + })); + + this.setState((prevState, currentProps) => { + this.fooBar(this.state.foo); + ~~~~~~~~~~~~~~ [1] + return { + bar: !prevState.bar + }; + }); + } +} + +[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. 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 + } +}