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
14.28k stars 442 forks source link

💅 Biome throw useExhaustiveDependencies error when useRef type is defined using type assertion #1597

Open nouman2075 opened 8 months ago

nouman2075 commented 8 months ago

Environment information

CLI:
  Version:                      1.5.2
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           linux

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v20.9.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "pnpm/8.10.2"

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://codesandbox.io/p/devbox/biome-shnrcy?file=%2Fsrc%2FApp.tsx

Expected result

If useRef type is define using type assertion and when ref is used in any react hook, exhaustive deps breaks.

Works fine

import { useEffect, useRef } from "react";

const App = () => {
  const ref = useRef<HTMLDivElement>();

  useEffect(() => {
    console.log(ref.current);
  }, []);

  return <div>Hello World</div>;
};

export default App;

Throw error This hook does not specify all of its dependencies.

import { type MutableRefObject, useEffect, useRef } from "react";

const App = () => {
  const ref = useRef() as MutableRefObject<HTMLDivElement>;

  useEffect(() => {
    console.log(ref.current);
  }, []);

  return <div>Hello World</div>;
};

export default App;

Code of Conduct

XiNiHa commented 7 months ago

I guess it's an edge case that was not handled correctly from https://github.com/biomejs/biome/pull/996. I'll look into it after fixing https://github.com/biomejs/biome/issues/1637.

XiNiHa commented 7 months ago

I'm not sure how many edge cases we should handle. Currently having these in mind:

There are many other kind of edge cases, like....

It's somewhat obvious that it is unrealistic to handle all of those edge cases. Therefore I believe we should scope which edge cases to handle. Personally, I think all of TS operator cases and array destructure should be handled, while other cases are too specific/niche to work on.

ematipico commented 7 months ago

I think so too. We should strive to cover the most used patterns and call it a day.

We should take this opportunity and update the documentation explaining the expectations of this rule

Sec-ant commented 7 months ago

Speaking of IIFE, there're also cases where one can use IIFE to return a function that can be used in useCallback or useEffect. This is a common functional pattern in JS to hoist and memoize static variables. The useExhaustiveDependencies rule doesn't catch it currently. It would be great to handle this kind of edge cases.

https://biomejs.dev/playground/?code=aQBtAHAAbwByAHQAIAB7ACAAdQBzAGUARQBmAGYAZQBjAHQALAAgAHUAcwBlAFMAdABhAHQAZQAgAH0AIABmAHIAbwBtACAAIgByAGUAYQBjAHQAIgA7AAoACgBlAHgAcABvAHIAdAAgAGYAdQBuAGMAdABpAG8AbgAgAEYAKAApACAAewAKAAoAIAAgAGMAbwBuAHMAdAAgAFsAYQAsACAAcwBlAHQAQQBdACAAPQAgAHUAcwBlAFMAdABhAHQAZQAoADEAKQA7AAoAIAAgAAoAIAAgAHUAcwBlAEUAZgBmAGUAYwB0ACgAKAApACAAPQA%2BACAAYwBvAG4AcwBvAGwAZQAuAGwAbwBnACgAYQApACwAIABbAF0AKQA7AAoACgAgACAAdQBzAGUARQBmAGYAZQBjAHQAKAAoACgAKQAgAD0APgAgACgAKQAgAD0APgAgAGMAbwBuAHMAbwBsAGUALgBsAG8AZwAoAGEAKQApACgAKQAsACAAWwBdACkAOwAKAH0A

mintthemoon commented 2 months ago

I ran into this yesterday, just found this issue unresolved so I figured I'd take a look at how eslint-plugin-react-hooks handles it. I use both in my project, and if I incorrectly include ref.current in my deps array as Biome wants me to, ESLint does catch it as unnecessary.

The way they handle this actually seems pretty clever and as far as I can tell cleanly solves at least a good majority of the edge cases you listed. They simply check if the value was returned from a useRef call: https://github.com/facebook/react/blob/163365a07872337e04826c4f501565d43dbd2fd4/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js#L251

I also found this on my way there, a lot of good discussion on this topic from when they developed their own exhaustive deps rule: https://github.com/facebook/react/issues/14920