Merge pull request #11 from Gelio/develop

Release v2.0.0

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 <div>Counter is over 5</div>;
+    }
+
+    useEffect(() => {
+      console.log('Counter is', counter);
+    });
+
+    return <div>{counter}</div>;
+  }
+  ```
+
 ## v1.1.0 (2019-02-09)
 
 - Allow using hooks inside React component decorators, such as `React.memo` or `React.forwardRef`.

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
 

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": {

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',
 };

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);
+}

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<ParentNodeType extends Node = Node>(
+  startingNode: Node,
+  predicate: TypeGuardPredicate<Node, ParentNodeType>,
+): ParentNodeType | null {
+  if (!startingNode.parent) {
+    return null;
+  }
+
+  if (predicate(startingNode.parent)) {
+    return startingNode.parent;
+  }
+
+  return findClosestAncestorNode(startingNode.parent, predicate);
+}

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));
+}

src/react-hooks-nesting-walker/is-react-api-expression.ts

@@ -5,7 +5,7 @@ import {
   Expression,
 } from 'typescript';
 
-export type Predicate<T> = (t: T) => boolean;
+import { Predicate } from './predicate';
 
 /**
  * Tests whether an `Expression` is an identifier that matches a predicate. Accepts also property

src/react-hooks-nesting-walker/predicate.ts

@@ -0,0 +1,3 @@
+export type Predicate<T> = (t: T) => boolean;
+
+export type TypeGuardPredicate<T, U extends T> = (t: T) => t is U;

src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts

@@ -14,15 +14,22 @@ import {
   isSourceFile,
   isClassDeclaration,
   isCallExpression,
+  ReturnStatement,
+  isReturnStatement,
 } from 'typescript';
 
 import { isHookCall } from './is-hook-call';
 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<FunctionNode>();
+
   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) &&

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 <div ref={useRef()}>Hello world</div>;
+                   ~~~~~~~~ [A hook should not appear after a return statement]
+}

test/tslint-rule/early-return.ts.lint

@@ -0,0 +1,38 @@
+function MyComponent() {
+  const [counter, setCounter] = useState(0);
+
+  if (counter > 5) {
+    return <div>Counter is over 5</div>;
+  }
+
+  useEffect(() => {
+  ~~~~~~~~~~~~~~~~~
+    console.log('Counter is', counter);
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  });
+~~~~ [A hook should not appear after a return statement]
+
+  return (
+    <div>
+      {counter} -
+      <button onClick={() => setCounter(counter + 1)}>+1</button>
+    </div>
+  );
+}
+
+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];
+}