biomejs / biome

A toolchain for web projects, aimed to provide functionalities to maintain them. Biome offers formatter and linter, usable via CLI and LSP.
https://biomejs.dev
Apache License 2.0
15.28k stars 475 forks source link

React hooks aren't linted if not matching very strict import pattern #1791

Closed samhh closed 6 months ago

samhh commented 9 months ago

Environment information

CLI:
  Version:                      1.5.3
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "screen"
  JS_RUNTIME_VERSION:           "v20.11.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         unset

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    false
  VCS disabled:                 true

Workspace:
  Open Documents:               0

Rule name

useExhaustiveDependencies

Playground link

https://biomejs.dev/playground/?code=aQBtAHAAbwByAHQAIAB7ACAAdQBzAGUARQBmAGYAZQBjAHQAIABhAHMAIAB1AHMAZQBFAGYAZgBlAGMAdABfACAAfQAgAGYAcgBvAG0AIAAiAHIAZQBhAGMAdAAiADsACgAKAGMAbwBuAHMAdAAgAHUAcwBlAEUAZgBmAGUAYwB0ACAAPQAgAHUAcwBlAEUAZgBmAGUAYwB0AF8AOwAKAAoALwAvACAAVABoAGkAcwAgAGkAcwAgAGwAaQBuAHQAZQBkACAAYQBzACAAZQB4AHAAZQBjAHQAZQBkAAoAZgB1AG4AYwB0AGkAbwBuACAAYwBvAG0AcABvAG4AZQBuAHQAKAApACAAewAKACAAIAAgACAAbABlAHQAIABhACAAPQAgADEAOwAKACAAIAAgACAAdQBzAGUARQBmAGYAZQBjAHQAKAAoACkAIAA9AD4AIAB7AAoAIAAgACAAIAAgACAAIAAgAGMAbwBuAHMAbwBsAGUALgBsAG8AZwAoAGEAKQA7AAoAIAAgACAAIAB9ACwAIABbAF0AKQA7AAoAfQA%3D

Expected result

In the linked playground the hook isn't linted because import was renamed before being used. This is a simplification of what led me here, which is that if you have a facade module that re-exports React hooks they also won't be linted.

Worse yet, if you add the following Biome config only useEffect2 will be linted - you can't encourage the linter to consider any other import path for useEffect, even with namespace imports:

{
  "linter": {
    "enabled": true,
    "rules": {
      "recommended": true,
      "correctness": {
        "useExhaustiveDependencies": {
          "level": "error",
          "options": {
            "hooks": [
              {
                "name": "useEffect",
                "closureIndex": 0,
                "dependenciesIndex": 1
              },
              {
                "name": "React.useEffect",
                "closureIndex": 0,
                "dependenciesIndex": 1
              },
              {
                "name": "useEffect2",
                "closureIndex": 0,
                "dependenciesIndex": 1
              }
            ]
          }
        }
      }
    }
  }
}
import * as React from './local-module.js';
const useEffect = React.useEffect;
const useEffect2 = useEffect;

export const C = (): void => {
  let a = 1;

  // This isn't linted, but it should be.
  React.useEffect(() => {
    a;
  }, []);

  // This isn't linted, but it should be.
  useEffect(() => {
    a;
  }, []);

  // This is linted, but we don't want to have to rename React's hooks when we
  // re-export them.
  useEffect2(() => {
    a;
  }, []);
};

This isn't an issue with ESLint's react-hooks/exhaustive-deps.

Code of Conduct

ematipico commented 9 months ago

"name": "React.useEffect",

This won't work, because we expect name to be a function call, not a static member expression, and I think we want to keep it this way; due to the nature of JavaScript, we must set some expectations, otherwise it would be impossible to retrieve the information we need.

samhh commented 6 months ago

Could we at least consider allowing custom hooks definitions to override the default ones if they share the same name?

arendjr commented 6 months ago

@samhh Doesn't this provide what you are looking for? https://biomejs.dev/linter/rules/use-exhaustive-dependencies/#options

samhh commented 6 months ago

Doesn't this provide what you are looking for? https://biomejs.dev/linter/rules/use-exhaustive-dependencies/#options

It seems something has changed since I raised this issue as this now works. Thanks for the prompt 🙂