From 9203fb9a8651f809712078abcb2a26e481d1d5c1 Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Thu, 7 Feb 2019 21:18:05 +0100 Subject: [PATCH 01/15] add a test case for detecting early-returns --- test/tslint-rule/early-return.ts.lint | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 test/tslint-rule/early-return.ts.lint diff --git a/test/tslint-rule/early-return.ts.lint b/test/tslint-rule/early-return.ts.lint new file mode 100644 index 0000000..96fe9cf --- /dev/null +++ b/test/tslint-rule/early-return.ts.lint @@ -0,0 +1,21 @@ +function MyComponent() { + const [counter, setCounter] = useState(0); + + if (counter > 5) { + return
Counter is over 5
; + } + + useEffect(() => { + ~~~~~~~~~~~~~~~~~ + console.log('Counter is', counter); +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + }); +~~~~ [A hook should not appear after a return statement] + + return ( +
+ {counter} - + +
+ ); +} From 7842c40ae1003f15eb70b2f45c5dccf2532dc1ac Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Sat, 9 Mar 2019 15:35:39 +0100 Subject: [PATCH 02/15] add test for detecting early returns in custom hooks --- test/tslint-rule/early-return.ts.lint | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/tslint-rule/early-return.ts.lint b/test/tslint-rule/early-return.ts.lint index 96fe9cf..7d39136 100644 --- a/test/tslint-rule/early-return.ts.lint +++ b/test/tslint-rule/early-return.ts.lint @@ -19,3 +19,12 @@ function MyComponent() { ); } + +function useCustomHook(param: number) { + if (param > 5) { + return 'a'; + } + + useEffect(() => { }); + ~~~~~~~~~~~~~~~~~~~~ [A hook should not appear after a return statement] +} From f9d68ddfd6d672d29d3f01448934be99c0a6028b Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Sat, 9 Mar 2019 15:36:26 +0100 Subject: [PATCH 03/15] add utility functions 1. Function nodes types and matchers 2. Finding parent functions --- .../find-parent-function.ts | 14 +++++++++++++ .../function-node.ts | 20 +++++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 src/react-hooks-nesting-walker/find-parent-function.ts create mode 100644 src/react-hooks-nesting-walker/function-node.ts diff --git a/src/react-hooks-nesting-walker/find-parent-function.ts b/src/react-hooks-nesting-walker/find-parent-function.ts new file mode 100644 index 0000000..b7678df --- /dev/null +++ b/src/react-hooks-nesting-walker/find-parent-function.ts @@ -0,0 +1,14 @@ +import { Node } from 'typescript'; +import { FunctionNode, isFunctionNode } from './function-node'; + +export function findParentFunction(node: Node): FunctionNode | null { + if (isFunctionNode(node)) { + return node; + } + + if (!node.parent) { + return null; + } + + return findParentFunction(node.parent); +} diff --git a/src/react-hooks-nesting-walker/function-node.ts b/src/react-hooks-nesting-walker/function-node.ts new file mode 100644 index 0000000..3c9c808 --- /dev/null +++ b/src/react-hooks-nesting-walker/function-node.ts @@ -0,0 +1,20 @@ +import { + FunctionDeclaration, + FunctionExpression, + ArrowFunction, + Node, + isFunctionDeclaration, + isFunctionExpression, + isArrowFunction, +} from 'typescript'; + +export type FunctionNode = + | FunctionDeclaration + | FunctionExpression + | ArrowFunction; + +const matchers = [isFunctionDeclaration, isFunctionExpression, isArrowFunction]; + +export function isFunctionNode(node: Node): node is FunctionNode { + return matchers.some(matcher => matcher(node)); +} From 025d13678848c596b14fe2491e5f165567bee972 Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Sat, 9 Mar 2019 15:36:41 +0100 Subject: [PATCH 04/15] add error message for detecting hooks after early returns --- src/react-hooks-nesting-walker/error-messages.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/react-hooks-nesting-walker/error-messages.ts b/src/react-hooks-nesting-walker/error-messages.ts index d8e1a96..6a40580 100644 --- a/src/react-hooks-nesting-walker/error-messages.ts +++ b/src/react-hooks-nesting-walker/error-messages.ts @@ -16,4 +16,5 @@ export const ERROR_MESSAGES = { invalidFunctionDeclaration: 'A hook cannot be used inside of another function', invalidFunctionExpression: 'A hook cannot be used inside of another function', + hookAfterEarlyReturn: 'A hook should not appear after a return statement', }; From c545a51c467bbceb1b2d366f791226f924048cb5 Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Sat, 9 Mar 2019 15:38:20 +0100 Subject: [PATCH 05/15] add detecting hooks after early returns --- .../react-hooks-nesting-walker.ts | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts b/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts index 243a156..9215ff4 100644 --- a/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts +++ b/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts @@ -14,6 +14,7 @@ import { isSourceFile, isClassDeclaration, isCallExpression, + ReturnStatement, } from 'typescript'; import { isHookCall } from './is-hook-call'; @@ -21,8 +22,12 @@ import { ERROR_MESSAGES } from './error-messages'; import { isBinaryConditionalExpression } from './is-binary-conditional-expression'; import { isComponentOrHookIdentifier } from './is-component-or-hook-identifier'; import { isReactComponentDecorator } from './is-react-component-decorator'; +import { findParentFunction } from './find-parent-function'; +import { FunctionNode } from './function-node'; export class ReactHooksNestingWalker extends RuleWalker { + private functionsWithReturnStatements = new Set(); + public visitCallExpression(node: CallExpression) { if (isHookCall(node)) { this.visitHookAncestor(node, node.parent); @@ -31,6 +36,16 @@ export class ReactHooksNestingWalker extends RuleWalker { super.visitCallExpression(node); } + public visitReturnStatement(node: ReturnStatement) { + const parentFunction = findParentFunction(node); + + if (parentFunction) { + this.functionsWithReturnStatements.add(parentFunction); + } + + super.visitReturnStatement(node); + } + private visitHookAncestor(hookNode: CallExpression, ancestor: Node) { /** * Fail for: @@ -78,6 +93,11 @@ export class ReactHooksNestingWalker extends RuleWalker { * } * ``` */ + + if (this.functionsWithReturnStatements.has(ancestor)) { + this.addFailureAtNode(hookNode, ERROR_MESSAGES.hookAfterEarlyReturn); + } + if (ancestor.name && isComponentOrHookIdentifier(ancestor.name)) { return; } @@ -102,6 +122,11 @@ export class ReactHooksNestingWalker extends RuleWalker { * } * ``` */ + + if (this.functionsWithReturnStatements.has(ancestor)) { + this.addFailureAtNode(hookNode, ERROR_MESSAGES.hookAfterEarlyReturn); + } + if ( isVariableDeclaration(ancestor.parent) && isIdentifier(ancestor.parent.name) && From 4220cc5e9e9e51bd05bdca2e1d915af542510aec Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Sat, 9 Mar 2019 15:51:12 +0100 Subject: [PATCH 06/15] add info about detecting early returns to the readme --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index c75e3e2..516d3c6 100644 --- a/README.md +++ b/README.md @@ -21,6 +21,7 @@ The rule is based on an [ESLint plugin for react hooks](https://github.com/faceb - ternary expressions - loops (`while`, `for`, `do ... while`) - functions that themselves are not custom hooks or components +- detects using React hooks in spite of an early return ## Installation From 4e66c3d9ffc4832caf4b26ed453297929bc4a323 Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Sat, 9 Mar 2019 15:56:37 +0100 Subject: [PATCH 07/15] describe detecting early returns in v2.0.0 in the changelog --- CHANGELOG.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8bd86b0..00d562a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,25 @@ # Changelog +## v2.0.0 (TBA) + +- Report violations whenever a React hook is used after an early return. + + For example, the following code sample now violates the rule: + + ```tsx + function MyComponent({ counter }) { + if (counter > 5) { + return
Counter is over 5
; + } + + useEffect(() => { + console.log('Counter is', counter); + }); + + return
{counter}
; + } + ``` + ## v1.1.0 (2019-02-09) - Allow using hooks inside React component decorators, such as `React.memo` or `React.forwardRef`. From d40f0a32e0931c0916211819c832b77ae7c23073 Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Sat, 9 Mar 2019 15:56:42 +0100 Subject: [PATCH 08/15] set version to 2.0.0-alpha.1 --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 5bf0f0e..47c5adb 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "tslint-react-hooks", - "version": "1.1.0", + "version": "2.0.0-alpha.1", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index c7cfed8..8d60629 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "tslint-react-hooks", - "version": "1.1.0", + "version": "2.0.0-alpha.1", "description": "TSLint rule that enforces the Rules of Hooks", "main": "tslint-react-hooks.json", "scripts": { From dc0c2f908ed723df886af122e6b8887a0c9ee6cf Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Mon, 11 Mar 2019 18:47:19 +0100 Subject: [PATCH 09/15] extract predicate types to a separate file --- src/react-hooks-nesting-walker/is-react-api-expression.ts | 2 +- src/react-hooks-nesting-walker/predicate.ts | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 src/react-hooks-nesting-walker/predicate.ts diff --git a/src/react-hooks-nesting-walker/is-react-api-expression.ts b/src/react-hooks-nesting-walker/is-react-api-expression.ts index 5720e9e..2f1ae59 100644 --- a/src/react-hooks-nesting-walker/is-react-api-expression.ts +++ b/src/react-hooks-nesting-walker/is-react-api-expression.ts @@ -5,7 +5,7 @@ import { Expression, } from 'typescript'; -export type Predicate = (t: T) => boolean; +import { Predicate } from './predicate'; /** * Tests whether an `Expression` is an identifier that matches a predicate. Accepts also property diff --git a/src/react-hooks-nesting-walker/predicate.ts b/src/react-hooks-nesting-walker/predicate.ts new file mode 100644 index 0000000..da022d6 --- /dev/null +++ b/src/react-hooks-nesting-walker/predicate.ts @@ -0,0 +1,3 @@ +export type Predicate = (t: T) => boolean; + +export type TypeGuardPredicate = (t: T) => t is U; From c83bbf8f69d6b1632dd505660238aa80ce00420a Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Mon, 11 Mar 2019 18:49:43 +0100 Subject: [PATCH 10/15] create a utility function for finding closest ancestor matching a predicate --- .../find-ancestor-function.ts | 8 +++++++ .../find-closest-ancestor-node.ts | 23 +++++++++++++++++++ .../find-parent-function.ts | 14 ----------- .../react-hooks-nesting-walker.ts | 4 ++-- 4 files changed, 33 insertions(+), 16 deletions(-) create mode 100644 src/react-hooks-nesting-walker/find-ancestor-function.ts create mode 100644 src/react-hooks-nesting-walker/find-closest-ancestor-node.ts delete mode 100644 src/react-hooks-nesting-walker/find-parent-function.ts diff --git a/src/react-hooks-nesting-walker/find-ancestor-function.ts b/src/react-hooks-nesting-walker/find-ancestor-function.ts new file mode 100644 index 0000000..2bbc976 --- /dev/null +++ b/src/react-hooks-nesting-walker/find-ancestor-function.ts @@ -0,0 +1,8 @@ +import { Node } from 'typescript'; + +import { FunctionNode, isFunctionNode } from './function-node'; +import { findClosestAncestorNode } from './find-closest-ancestor-node'; + +export function findAncestorFunction(node: Node): FunctionNode | null { + return findClosestAncestorNode(node, isFunctionNode); +} diff --git a/src/react-hooks-nesting-walker/find-closest-ancestor-node.ts b/src/react-hooks-nesting-walker/find-closest-ancestor-node.ts new file mode 100644 index 0000000..f73492b --- /dev/null +++ b/src/react-hooks-nesting-walker/find-closest-ancestor-node.ts @@ -0,0 +1,23 @@ +import { Node } from 'typescript'; + +import { TypeGuardPredicate } from './predicate'; + +/** + * Finds a closest ancestor that matches a given predicate. + * + * Ensures type safety + */ +export function findClosestAncestorNode( + startingNode: Node, + predicate: TypeGuardPredicate, +): ParentNodeType | null { + if (!startingNode.parent) { + return null; + } + + if (predicate(startingNode.parent)) { + return startingNode.parent; + } + + return findClosestAncestorNode(startingNode.parent, predicate); +} diff --git a/src/react-hooks-nesting-walker/find-parent-function.ts b/src/react-hooks-nesting-walker/find-parent-function.ts deleted file mode 100644 index b7678df..0000000 --- a/src/react-hooks-nesting-walker/find-parent-function.ts +++ /dev/null @@ -1,14 +0,0 @@ -import { Node } from 'typescript'; -import { FunctionNode, isFunctionNode } from './function-node'; - -export function findParentFunction(node: Node): FunctionNode | null { - if (isFunctionNode(node)) { - return node; - } - - if (!node.parent) { - return null; - } - - return findParentFunction(node.parent); -} diff --git a/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts b/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts index 9215ff4..450167c 100644 --- a/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts +++ b/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts @@ -22,7 +22,7 @@ import { ERROR_MESSAGES } from './error-messages'; import { isBinaryConditionalExpression } from './is-binary-conditional-expression'; import { isComponentOrHookIdentifier } from './is-component-or-hook-identifier'; import { isReactComponentDecorator } from './is-react-component-decorator'; -import { findParentFunction } from './find-parent-function'; +import { findAncestorFunction } from './find-ancestor-function'; import { FunctionNode } from './function-node'; export class ReactHooksNestingWalker extends RuleWalker { @@ -37,7 +37,7 @@ export class ReactHooksNestingWalker extends RuleWalker { } public visitReturnStatement(node: ReturnStatement) { - const parentFunction = findParentFunction(node); + const parentFunction = findAncestorFunction(node); if (parentFunction) { this.functionsWithReturnStatements.add(parentFunction); From 978e5ebb6dc863fe63b1a40f1622afab63c909e0 Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Mon, 11 Mar 2019 18:50:16 +0100 Subject: [PATCH 11/15] add test cases for calling hooks in return statements --- test/not-covered-cases/use-hook-in-return.ts.lint | 7 +++++++ test/tslint-rule/early-return.ts.lint | 8 ++++++++ 2 files changed, 15 insertions(+) create mode 100644 test/not-covered-cases/use-hook-in-return.ts.lint diff --git a/test/not-covered-cases/use-hook-in-return.ts.lint b/test/not-covered-cases/use-hook-in-return.ts.lint new file mode 100644 index 0000000..badcfeb --- /dev/null +++ b/test/not-covered-cases/use-hook-in-return.ts.lint @@ -0,0 +1,7 @@ +function MyComponentReturningHookCall() { + // Using a hook inside of a returned value should not be a rule violation, + // even if it looks silly. + + return
Hello world
; + ~~~~~~~~ [A hook should not appear after a return statement] +} diff --git a/test/tslint-rule/early-return.ts.lint b/test/tslint-rule/early-return.ts.lint index 7d39136..ca500c0 100644 --- a/test/tslint-rule/early-return.ts.lint +++ b/test/tslint-rule/early-return.ts.lint @@ -28,3 +28,11 @@ function useCustomHook(param: number) { useEffect(() => { }); ~~~~~~~~~~~~~~~~~~~~ [A hook should not appear after a return statement] } + + +// ============================= +// Do not report rule violations if the hook is called in the `return` statement + +function useMyHook() { + return useState(true)[0]; +} From 175e8dca1bf42dbe3346868e874b9518e71a2c63 Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Mon, 11 Mar 2019 18:51:35 +0100 Subject: [PATCH 12/15] stop reporting violations for hooks called in return statements --- .../react-hooks-nesting-walker.ts | 34 +++++++++++++++++-- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts b/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts index 450167c..3ed77cc 100644 --- a/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts +++ b/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts @@ -15,6 +15,7 @@ import { isClassDeclaration, isCallExpression, ReturnStatement, + isReturnStatement, } from 'typescript'; import { isHookCall } from './is-hook-call'; @@ -23,7 +24,8 @@ import { isBinaryConditionalExpression } from './is-binary-conditional-expressio import { isComponentOrHookIdentifier } from './is-component-or-hook-identifier'; import { isReactComponentDecorator } from './is-react-component-decorator'; import { findAncestorFunction } from './find-ancestor-function'; -import { FunctionNode } from './function-node'; +import { FunctionNode, isFunctionNode } from './function-node'; +import { findClosestAncestorNode } from './find-closest-ancestor-node'; export class ReactHooksNestingWalker extends RuleWalker { private functionsWithReturnStatements = new Set(); @@ -95,7 +97,18 @@ export class ReactHooksNestingWalker extends RuleWalker { */ if (this.functionsWithReturnStatements.has(ancestor)) { - this.addFailureAtNode(hookNode, ERROR_MESSAGES.hookAfterEarlyReturn); + const closestReturnStatementOrFunctionNode = findClosestAncestorNode( + hookNode, + (node): node is ReturnStatement | FunctionNode => + isReturnStatement(node) || isFunctionNode(node), + ); + + if ( + closestReturnStatementOrFunctionNode && + !isReturnStatement(closestReturnStatementOrFunctionNode) + ) { + this.addFailureAtNode(hookNode, ERROR_MESSAGES.hookAfterEarlyReturn); + } } if (ancestor.name && isComponentOrHookIdentifier(ancestor.name)) { @@ -123,8 +136,23 @@ export class ReactHooksNestingWalker extends RuleWalker { * ``` */ + /** + * REFACTOR: Use a shared implementation for all types of functions. + * The logic below is duplicated for function declarations. + */ if (this.functionsWithReturnStatements.has(ancestor)) { - this.addFailureAtNode(hookNode, ERROR_MESSAGES.hookAfterEarlyReturn); + const closestReturnStatementOrFunctionNode = findClosestAncestorNode( + hookNode, + (node): node is ReturnStatement | FunctionNode => + isReturnStatement(node) || isFunctionNode(node), + ); + + if ( + closestReturnStatementOrFunctionNode && + !isReturnStatement(closestReturnStatementOrFunctionNode) + ) { + this.addFailureAtNode(hookNode, ERROR_MESSAGES.hookAfterEarlyReturn); + } } if ( From 3a714d794cf14e71907f9b8b9dd3a268d5b642c8 Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Mon, 11 Mar 2019 18:52:13 +0100 Subject: [PATCH 13/15] update version to v2.0.0-alpha.2 --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 47c5adb..d9dc5a5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "tslint-react-hooks", - "version": "2.0.0-alpha.1", + "version": "2.0.0-alpha.2", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 8d60629..c703bff 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "tslint-react-hooks", - "version": "2.0.0-alpha.1", + "version": "2.0.0-alpha.2", "description": "TSLint rule that enforces the Rules of Hooks", "main": "tslint-react-hooks.json", "scripts": { From ff493226b45b9d66f2d93900db34d06ea9d28bd3 Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Tue, 12 Mar 2019 21:56:25 +0100 Subject: [PATCH 14/15] update version to v2.0.0 --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index d9dc5a5..d551c25 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "tslint-react-hooks", - "version": "2.0.0-alpha.2", + "version": "2.0.0", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index c703bff..b1ab7c3 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "tslint-react-hooks", - "version": "2.0.0-alpha.2", + "version": "2.0.0", "description": "TSLint rule that enforces the Rules of Hooks", "main": "tslint-react-hooks.json", "scripts": { From 6a6cb4d38e655901e724fc90e729d2c42a051f1a Mon Sep 17 00:00:00 2001 From: Grzegorz Rozdzialik Date: Tue, 12 Mar 2019 21:56:47 +0100 Subject: [PATCH 15/15] set release date of v2.0.0 in the changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 00d562a..c6a7fe5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## v2.0.0 (TBA) +## v2.0.0 (2019-03-12) - Report violations whenever a React hook is used after an early return.