diff --git a/.gitignore b/.gitignore index 12db4fa..c7d51dd 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,9 @@ # See http://help.github.com/ignore-files/ for more about ignoring files. +# TSLint generated +*.js +*.js.map + # dependencies node_modules diff --git a/.idea/codeStyleSettings.xml b/.idea/codeStyleSettings.xml new file mode 100644 index 0000000..dadb2e2 --- /dev/null +++ b/.idea/codeStyleSettings.xml @@ -0,0 +1,29 @@ + + + + + + \ No newline at end of file diff --git a/src/Sample.js b/src/Sample.js deleted file mode 100644 index 71e8e72..0000000 --- a/src/Sample.js +++ /dev/null @@ -1,20 +0,0 @@ -var Class = (function () { - function Class() { - this.setState({ obj: 123 }); - this.setState({ obj: 123 }, function () { return "callback"; }); - this.setState(function (_) { return ({ obj: 123 }); }); - this.setState(function (prevState) { return ({ obj: prevState.obj }); }); - this.setState(function (prevState, props) { return ({ obj: prevState.obj, prop: props.obj }); }); - this.setState(function (_, props) { return ({ prop: props.obj }); }); - this.setState(function (_) { return ({ obj: 123 }); }, function () { return "callback"; }); - this.setState(function (prevState) { return ({ obj: prevState.obj }); }, function () { return "callback"; }); - this.setState(function (prevState, props) { return ({ obj: prevState.obj, prop: props.obj }); }, function () { return "callback"; }); - this.setState(function (_, props) { return ({ prop: props.obj }); }, function () { return "callback"; }); - } - Class.prototype.setState = function (arg, callback) { - arg = callback && callback(); - return arg; - }; - return Class; -}()); -//# sourceMappingURL=Test.js.map \ No newline at end of file diff --git a/src/Sample.js.map b/src/Sample.js.map deleted file mode 100644 index 2f30281..0000000 --- a/src/Sample.js.map +++ /dev/null @@ -1 +0,0 @@ -{"version":3,"file":"Test.js","sourceRoot":"","sources":["Test.tsx"],"names":[],"mappings":"AAAA;IACI;QACI,IAAI,CAAC,QAAQ,CAAC,EAAC,GAAG,EAAE,GAAG,EAAC,CAAC,CAAC;QAC1B,IAAI,CAAC,QAAQ,CAAC,EAAC,GAAG,EAAE,GAAG,EAAC,EAAE,cAAM,OAAA,UAAU,EAAV,CAAU,CAAC,CAAC;QAE5C,IAAI,CAAC,QAAQ,CAAC,UAAC,CAAC,IAAK,OAAA,CAAC,EAAC,GAAG,EAAE,GAAG,EAAC,CAAC,EAAZ,CAAY,CAAC,CAAC;QACnC,IAAI,CAAC,QAAQ,CAAC,UAAC,SAAS,IAAK,OAAA,CAAC,EAAC,GAAG,EAAE,SAAS,CAAC,GAAG,EAAC,CAAC,EAAtB,CAAsB,CAAC,CAAC;QACrD,IAAI,CAAC,QAAQ,CAAC,UAAC,SAAS,EAAE,KAAK,IAAK,OAAA,CAAC,EAAC,GAAG,EAAE,SAAS,CAAC,GAAG,EAAE,IAAI,EAAE,KAAK,CAAC,GAAG,EAAC,CAAC,EAAvC,CAAuC,CAAC,CAAC;QAC7E,IAAI,CAAC,QAAQ,CAAC,UAAC,CAAC,EAAE,KAAK,IAAK,OAAA,CAAC,EAAC,IAAI,EAAE,KAAK,CAAC,GAAG,EAAC,CAAC,EAAnB,CAAmB,CAAC,CAAC;QAEjD,IAAI,CAAC,QAAQ,CAAC,UAAC,CAAC,IAAK,OAAA,CAAC,EAAC,GAAG,EAAE,GAAG,EAAC,CAAC,EAAZ,CAAY,EAAE,cAAM,OAAA,UAAU,EAAV,CAAU,CAAC,CAAC;QACrD,IAAI,CAAC,QAAQ,CAAC,UAAC,SAAS,IAAK,OAAA,CAAC,EAAC,GAAG,EAAE,SAAS,CAAC,GAAG,EAAC,CAAC,EAAtB,CAAsB,EAAE,cAAM,OAAA,UAAU,EAAV,CAAU,CAAC,CAAC;QACvE,IAAI,CAAC,QAAQ,CAAC,UAAC,SAAS,EAAE,KAAK,IAAK,OAAA,CAAC,EAAC,GAAG,EAAE,SAAS,CAAC,GAAG,EAAE,IAAI,EAAE,KAAK,CAAC,GAAG,EAAC,CAAC,EAAvC,CAAuC,EAAE,cAAM,OAAA,UAAU,EAAV,CAAU,CAAC,CAAC;QAC/F,IAAI,CAAC,QAAQ,CAAC,UAAC,CAAC,EAAE,KAAK,IAAK,OAAA,CAAC,EAAC,IAAI,EAAE,KAAK,CAAC,GAAG,EAAC,CAAC,EAAnB,CAAmB,EAAE,cAAM,OAAA,UAAU,EAAV,CAAU,CAAC,CAAC;IACvE,CAAC;IAEO,wBAAQ,GAAhB,UAAiB,GAAG,EAAE,QAAS;QAC3B,GAAG,GAAG,QAAQ,IAAI,QAAQ,EAAE,CAAC;QAC7B,MAAM,CAAC,GAAG,CAAC;IACf,CAAC;IACL,YAAC;AAAD,CAAC,AApBD,IAoBC"} \ No newline at end of file diff --git a/src/Sample.tsx b/src/Sample.tsx index cde90a4..85b2f49 100644 --- a/src/Sample.tsx +++ b/src/Sample.tsx @@ -1,8 +1,32 @@ +import {isNullOrUndefined} from "util"; class Class { + private state = { + flag: true, + doNotAccessMeInSetState: () => null, + }; + + private props = { + doNotAccessMeInSetState: true, + }; + constructor() { + // Use functional setState instead + this.setState({obj: 123}); this.setState({obj: 123}); this.setState({obj: 123}, () => "callback"); + // Do not access this.props nor this.state in setState + this.setState((_) => ({obj: 123, prop: this.props.doNotAccessMeInSetState})); + this.setState((_) => ({obj: 123, flag: !this.state.flag})); + this.setState((_) => ({obj: 123, prop: this.props.doNotAccessMeInSetState, state: this.state.doNotAccessMeInSetState}), () => "callback"); + + this.setState(({obj: 123, prop: this.props.doNotAccessMeInSetState, state: this.state.doNotAccessMeInSetState}), () => "callback"); + + // This will probably be a false negative + const x = { flag: this.state.flag }; + this.setState(x); + + // These are okay this.setState((_) => ({obj: 123})); this.setState((prevState) => ({obj: prevState.obj})); this.setState((prevState, props) => ({obj: prevState.obj, prop: props.obj})); diff --git a/src/rules/reactSetStateUsageOptions.ts b/src/rules/reactSetStateUsageOptions.ts new file mode 100644 index 0000000..1a63c43 --- /dev/null +++ b/src/rules/reactSetStateUsageOptions.ts @@ -0,0 +1,13 @@ +export const OPTION_UPDATER_ONLY = "updater-only"; + +export interface IOptions { + readonly updaterOnly: boolean; +} + +export function parseOptions(ruleArguments: any[]): IOptions { + const updaterOnly = ruleArguments[0] as string; + + return { + updaterOnly: updaterOnly === OPTION_UPDATER_ONLY, + }; +} diff --git a/src/rules/reactSetStateUsageRule.ts b/src/rules/reactSetStateUsageRule.ts index 715465a..c4d114b 100644 --- a/src/rules/reactSetStateUsageRule.ts +++ b/src/rules/reactSetStateUsageRule.ts @@ -1,16 +1,26 @@ -import * as Lint from "tslint"; -import {isCallExpression, isPropertyAccessExpression} from "tsutils"; import * as ts from "typescript"; +import * as Lint from "tslint"; +import { isObjectLiteralExpression } from "tsutils"; -const OPTION_UPDATER_ONLY = "updater-only"; +import { + IOptions, + OPTION_UPDATER_ONLY, + parseOptions, +} from "./reactSetStateUsageOptions"; +import { + getFirstSetStateAncestor, + isThisPropertyAccess, + isThisSetState, + removeParentheses, +} from "../utils/syntaxWalkerUtils"; -interface IOptions { - readonly updaterOnly: boolean; -} +const FAILURE_STRING = "Do not pass an object into setState. Use functional setState updater instead."; +const FAILURE_STRING_UPDATER_ONLY = `Do not use callback parameter in setState. Use componentDidUpdate method instead (\"${OPTION_UPDATER_ONLY}\" switch).`; +const getFailureStringForAccessedMember = (accessedMember: string) => `Do not access 'this.${accessedMember}' in setState. Use arguments from callback function instead.`; export class Rule extends Lint.Rules.AbstractRule { public static metadata: Lint.IRuleMetadata = { - description: "Requires the setState function to be called with function as the first argument.", + description: "Requires the setState function to be called with function as the first argument and without 'this.props' nor 'this.state' access within the function.", optionExamples: [true], options: { items: [ @@ -29,36 +39,52 @@ export class Rule extends Lint.Rules.AbstractRule { typescriptOnly: false, }; - public static FAILURE_STRING = "Use functional setState instead of passing an object."; - public static FAILURE_STRING_UPDATER_ONLY = "Do not use callback parameter \"updater-only\" switch"; - public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { const options = parseOptions(this.ruleArguments); return this.applyWithFunction(sourceFile, walk, options); } +} + +function walk(ctx: Lint.WalkContext) { + const { sourceFile, options: { updaterOnly } } = ctx; + + function cb(node: ts.Node): void { + if (isThisSetState(node)) { + inspectSetStateCall(node, ctx, updaterOnly); + } + else if (isThisState(node) || isThisProps(node)) { + inspectThisPropsOrStateContext(node, ctx); + } + + return ts.forEachChild(node, cb); + } + return ts.forEachChild(sourceFile, cb); } -function parseOptions(ruleArguments: any[]): IOptions { - const updaterOnly = ruleArguments[0] as string; +function inspectSetStateCall(node: ts.CallExpression, ctx: Lint.WalkContext, updaterOnly: boolean) { + const { 0: updaterArgument, 1: callbackArgument, length: argumentsCount } = node.arguments; - return { - updaterOnly: updaterOnly === OPTION_UPDATER_ONLY, - }; + // Forbid object literal + const bareUpdaterArgument = removeParentheses(updaterArgument); + if (isObjectLiteralExpression(bareUpdaterArgument)) { + ctx.addFailureAtNode(updaterArgument, FAILURE_STRING); + } + + // Forbid second argument if updaterOnly flag set + if (updaterOnly && argumentsCount > 1) { + ctx.addFailureAtNode(callbackArgument, FAILURE_STRING_UPDATER_ONLY); + } } -function walk(ctx: Lint.WalkContext) { - const {options: {updaterOnly}} = ctx; - return ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void { - if (isCallExpression(node) && isPropertyAccessExpression(node.expression) && - node.expression.name.text === "setState") { - if (node.arguments.some((arg) => arg.kind === ts.SyntaxKind.ObjectLiteralExpression)) { - ctx.addFailureAtNode(node.arguments[0], Rule.FAILURE_STRING); - } - if (updaterOnly && node.arguments.length > 1) { - ctx.addFailureAtNode(node.arguments[1], Rule.FAILURE_STRING_UPDATER_ONLY); - } - } - return ts.forEachChild(node, cb); - }); +function inspectThisPropsOrStateContext(node: ts.PropertyAccessExpression, ctx: Lint.WalkContext) { + const setStateCall = getFirstSetStateAncestor(node.parent); + + if (setStateCall) { + ctx.addFailureAtNode(node, getFailureStringForAccessedMember(node.name.text)); + } } + +const isThisState = (node: ts.Node): node is ts.PropertyAccessExpression => isThisPropertyAccess(node) && node.name.text === "state"; + +const isThisProps = (node: ts.Node): node is ts.PropertyAccessExpression => isThisPropertyAccess(node) && node.name.text === "props"; diff --git a/src/utils/syntaxKindUtils.ts b/src/utils/syntaxKindUtils.ts new file mode 100644 index 0000000..9f0f349 --- /dev/null +++ b/src/utils/syntaxKindUtils.ts @@ -0,0 +1,3 @@ +import * as ts from "typescript"; + +export const isThisKeyword = (expression: ts.Expression) => expression.kind === ts.SyntaxKind.ThisKeyword; diff --git a/src/utils/syntaxWalkerUtils.ts b/src/utils/syntaxWalkerUtils.ts new file mode 100644 index 0000000..7c61d05 --- /dev/null +++ b/src/utils/syntaxWalkerUtils.ts @@ -0,0 +1,33 @@ +import * as ts from "typescript"; +import { + isCallExpression, + isPropertyAccessExpression, + isParenthesizedExpression, +} from "tsutils"; + +import { isThisKeyword } from "./syntaxKindUtils"; + +export const isThisPropertyAccess = (node: ts.Node): node is ts.PropertyAccessExpression => + isPropertyAccessExpression(node) && isThisKeyword(node.expression); + +export const isThisSetState = (node: ts.Node): node is ts.CallExpression => + isCallExpression(node) + && isPropertyAccessExpression(node.expression) + && isThisKeyword(node.expression.expression) + && node.expression.name.text === "setState"; + +export function getFirstSetStateAncestor(node: ts.Node): ts.CallExpression { + if (node.kind === ts.SyntaxKind.SourceFile) { + return null; + } + + if (isThisSetState(node)) { + return node as ts.CallExpression; + } + + return getFirstSetStateAncestor(node.parent); +} + +export const removeParentheses = (node: ts.Node) => isParenthesizedExpression(node) + ? removeParentheses(node.expression) + : node; diff --git a/test/rules/react-set-state-usage/default/test.ts.lint b/test/rules/react-set-state-usage/default/test.ts.lint index 70b9bd1..61cf989 100644 --- a/test/rules/react-set-state-usage/default/test.ts.lint +++ b/test/rules/react-set-state-usage/default/test.ts.lint @@ -1,7 +1,7 @@ this.setState({obj: 123}); - ~~~~~~~~~~ [Use functional setState instead of passing an object.] + ~~~~~~~~~~ [Do not pass an object into setState. Use functional setState updater instead.] this.setState({obj: 123}, () => "callback"); - ~~~~~~~~~~ [Use functional setState instead of passing an object.] + ~~~~~~~~~~ [Do not pass an object into setState. Use functional setState updater instead.] this.setState((_) => ({obj: 123})); this.setState((prevState) => ({obj: prevState.obj})); @@ -12,3 +12,23 @@ this.setState((_) => ({obj: 123}), () => "callback"); this.setState((prevState) => ({obj: prevState.obj}), () => "callback"); this.setState((prevState, props) => ({obj: prevState.obj, prop: props.obj}), () => "callback"); this.setState((_, props) => ({prop: props.obj}), () => "callback"); + +this.setState((_) => ({obj: 123, prop: this.props.doNotAccessMeInSetState})); + ~~~~~~~~~~ [Do not access 'this.props' in setState. Use arguments from callback function instead.] + +this.setState((_) => ({obj: 123, prop: this.state.doNotAccessMeInSetState})); + ~~~~~~~~~~ [Do not access 'this.state' in setState. Use arguments from callback function instead.] + +this.setState((_) => ({obj: 123, prop: this.props.doNotAccessMeInSetState, state: this.state.doNotAccessMeInSetState})); + ~~~~~~~~~~ [Do not access 'this.props' in setState. Use arguments from callback function instead.] + ~~~~~~~~~~ [Do not access 'this.state' in setState. Use arguments from callback function instead.] + +this.setState(({obj: 123, prop: this.props.doNotAccessMeInSetState, state: this.state.doNotAccessMeInSetState}))); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Do not pass an object into setState. Use functional setState updater instead.] + ~~~~~~~~~~ [Do not access 'this.props' in setState. Use arguments from callback function instead.] + ~~~~~~~~~~ [Do not access 'this.state' in setState. Use arguments from callback function instead.] + +this.setState(((({obj: 123, prop: this.props.doNotAccessMeInSetState, state: this.state.doNotAccessMeInSetState}))))); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Do not pass an object into setState. Use functional setState updater instead.] + ~~~~~~~~~~ [Do not access 'this.props' in setState. Use arguments from callback function instead.] + ~~~~~~~~~~ [Do not access 'this.state' in setState. Use arguments from callback function instead.] diff --git a/test/rules/react-set-state-usage/updaterOnly/test.ts.lint b/test/rules/react-set-state-usage/updaterOnly/test.ts.lint index 2d02f44..434f146 100644 --- a/test/rules/react-set-state-usage/updaterOnly/test.ts.lint +++ b/test/rules/react-set-state-usage/updaterOnly/test.ts.lint @@ -1,18 +1,49 @@ this.setState({obj: 123}); - ~~~~~~~~~~ [Use functional setState instead of passing an object.] + ~~~~~~~~~~ [Do not pass an object into setState. Use functional setState updater instead.] this.setState({obj: 123}, () => "callback"); - ~~~~~~~~~~ [Use functional setState instead of passing an object.] - ~~~~~~~~~~~~~~~~ [Do not use callback parameter "updater-only" switch] + ~~~~~~~~~~ [Do not pass an object into setState. Use functional setState updater instead.] + ~~~~~~~~~~~~~~~~ [Do not use callback parameter in setState. Use componentDidUpdate method instead ("updater-only" switch).] this.setState((_) => ({obj: 123}), () => "callback"); - ~~~~~~~~~~~~~~~~ [Do not use callback parameter "updater-only" switch] + ~~~~~~~~~~~~~~~~ [Do not use callback parameter in setState. Use componentDidUpdate method instead ("updater-only" switch).] this.setState((prevState) => ({obj: prevState.obj}), () => "callback"); - ~~~~~~~~~~~~~~~~ [Do not use callback parameter "updater-only" switch] + ~~~~~~~~~~~~~~~~ [Do not use callback parameter in setState. Use componentDidUpdate method instead ("updater-only" switch).] this.setState((prevState, props) => ({obj: prevState.obj, prop: props.obj}), () => "callback"); - ~~~~~~~~~~~~~~~~ [Do not use callback parameter "updater-only" switch] + ~~~~~~~~~~~~~~~~ [Do not use callback parameter in setState. Use componentDidUpdate method instead ("updater-only" switch).] this.setState((_, props) => ({prop: props.obj}), () => "callback"); - ~~~~~~~~~~~~~~~~ [Do not use callback parameter "updater-only" switch] + ~~~~~~~~~~~~~~~~ [Do not use callback parameter in setState. Use componentDidUpdate method instead ("updater-only" switch).] this.setState((_) => ({obj: 123})); this.setState((prevState) => ({obj: prevState.obj})); this.setState((prevState, props) => ({obj: prevState.obj, prop: props.obj})); this.setState((_, props) => ({prop: props.obj})); + +this.setState((_) => ({obj: 123, prop: this.props.doNotAccessMeInSetState})); + ~~~~~~~~~~ [Do not access 'this.props' in setState. Use arguments from callback function instead.] + +this.setState((_) => ({obj: 123, prop: this.state.doNotAccessMeInSetState})); + ~~~~~~~~~~ [Do not access 'this.state' in setState. Use arguments from callback function instead.] + +this.setState((_) => ({obj: 123, prop: this.props.doNotAccessMeInSetState}), () => "callback"); + ~~~~~~~~~~ [Do not access 'this.props' in setState. Use arguments from callback function instead.] + ~~~~~~~~~~~~~~~~ [Do not use callback parameter in setState. Use componentDidUpdate method instead ("updater-only" switch).] + +this.setState((_) => ({obj: 123, prop: this.state.doNotAccessMeInSetState}), () => "callback"); + ~~~~~~~~~~ [Do not access 'this.state' in setState. Use arguments from callback function instead.] + ~~~~~~~~~~~~~~~~ [Do not use callback parameter in setState. Use componentDidUpdate method instead ("updater-only" switch).] + +this.setState((_) => ({obj: 123, prop: this.props.doNotAccessMeInSetState, state: this.state.doNotAccessMeInSetState}), () => "callback"); + ~~~~~~~~~~ [Do not access 'this.props' in setState. Use arguments from callback function instead.] + ~~~~~~~~~~ [Do not access 'this.state' in setState. Use arguments from callback function instead.] + ~~~~~~~~~~~~~~~~ [Do not use callback parameter in setState. Use componentDidUpdate method instead ("updater-only" switch).] + +this.setState(({obj: 123, prop: this.props.doNotAccessMeInSetState, state: this.state.doNotAccessMeInSetState}), () => "callback"); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Do not pass an object into setState. Use functional setState updater instead.] + ~~~~~~~~~~ [Do not access 'this.props' in setState. Use arguments from callback function instead.] + ~~~~~~~~~~ [Do not access 'this.state' in setState. Use arguments from callback function instead.] + ~~~~~~~~~~~~~~~~ [Do not use callback parameter in setState. Use componentDidUpdate method instead ("updater-only" switch).] + +this.setState(((({obj: 123, prop: this.props.doNotAccessMeInSetState, state: this.state.doNotAccessMeInSetState}))), () => "callback"); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Do not pass an object into setState. Use functional setState updater instead.] + ~~~~~~~~~~ [Do not access 'this.props' in setState. Use arguments from callback function instead.] + ~~~~~~~~~~ [Do not access 'this.state' in setState. Use arguments from callback function instead.] + ~~~~~~~~~~~~~~~~ [Do not use callback parameter in setState. Use componentDidUpdate method instead ("updater-only" switch).]