diff --git a/CHANGELOG.md b/CHANGELOG.md index 8bd86b0..c6a7fe5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,25 @@ # Changelog +## v2.0.0 (2019-03-12) + +- 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`. 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 diff --git a/package-lock.json b/package-lock.json index 5bf0f0e..d551c25 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", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index c7cfed8..b1ab7c3 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "tslint-react-hooks", - "version": "1.1.0", + "version": "2.0.0", "description": "TSLint rule that enforces the Rules of Hooks", "main": "tslint-react-hooks.json", "scripts": { 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', }; 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/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)); +} 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; 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..3ed77cc 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,8 @@ import { isSourceFile, isClassDeclaration, isCallExpression, + ReturnStatement, + isReturnStatement, } from 'typescript'; import { isHookCall } from './is-hook-call'; @@ -21,8 +23,13 @@ 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 { findAncestorFunction } from './find-ancestor-function'; +import { FunctionNode, isFunctionNode } from './function-node'; +import { findClosestAncestorNode } from './find-closest-ancestor-node'; export class ReactHooksNestingWalker extends RuleWalker { + private functionsWithReturnStatements = new Set(); + public visitCallExpression(node: CallExpression) { if (isHookCall(node)) { this.visitHookAncestor(node, node.parent); @@ -31,6 +38,16 @@ export class ReactHooksNestingWalker extends RuleWalker { super.visitCallExpression(node); } + public visitReturnStatement(node: ReturnStatement) { + const parentFunction = findAncestorFunction(node); + + if (parentFunction) { + this.functionsWithReturnStatements.add(parentFunction); + } + + super.visitReturnStatement(node); + } + private visitHookAncestor(hookNode: CallExpression, ancestor: Node) { /** * Fail for: @@ -78,6 +95,22 @@ export class ReactHooksNestingWalker extends RuleWalker { * } * ``` */ + + if (this.functionsWithReturnStatements.has(ancestor)) { + 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)) { return; } @@ -102,6 +135,26 @@ 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)) { + const closestReturnStatementOrFunctionNode = findClosestAncestorNode( + hookNode, + (node): node is ReturnStatement | FunctionNode => + isReturnStatement(node) || isFunctionNode(node), + ); + + if ( + closestReturnStatementOrFunctionNode && + !isReturnStatement(closestReturnStatementOrFunctionNode) + ) { + this.addFailureAtNode(hookNode, ERROR_MESSAGES.hookAfterEarlyReturn); + } + } + if ( isVariableDeclaration(ancestor.parent) && isIdentifier(ancestor.parent.name) && 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 new file mode 100644 index 0000000..ca500c0 --- /dev/null +++ b/test/tslint-rule/early-return.ts.lint @@ -0,0 +1,38 @@ +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} - + +
+ ); +} + +function useCustomHook(param: number) { + if (param > 5) { + return 'a'; + } + + 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]; +}