🐛 `useHookAtTopLevel` doesn't report hooks used outside of non-hook functions

[itsMapleLeaf]

Environment information

CLI:
  Version:                      1.5.3
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           windows

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         unset
  JS_RUNTIME_VERSION:           "v21.6.2"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "pnpm/8.15.1"

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

Workspace:
  Open Documents:               0

What happened?

In this code:

function notAhook() {
    useSomeHook()
}

useHookAtTopLevel does not show an error for useSomeHook not being used inside a component-named or hook-named function.

Playground link

Expected result

useHookAtTopLevel should show an error for useSomeHook not being used inside a component-named or hook-named function.

References:

• https://react.dev/learn/state-a-components-memory#meet-your-first-hook
• https://legacy.reactjs.org/docs/hooks-rules.html#only-call-hooks-from-react-functions

Code of Conduct

• [X] I agree to follow Biome's Code of Conduct

[arendjr]

I’m not entirely sure how to fix this, since we don’t want to show diagnostics when a hook is called from inside a test function, for instance. Do you know how the ESLint plugin handles this?

[itsMapleLeaf]

we don’t want to show diagnostics when a hook is called from inside a test function

Sorry, I'm not sure what you mean by this. Can you show an example? I'm imagining something like this:

test("something", () => {
    const value = useWhatever()
})

...which isn't valid anyways 🤔

[arendjr]

You’re pretty close :)

For instance:

import { renderHook } from '@testing-library/react';
import useCustomHook from './useCustomHook';

test('should do something', () => {
  const { result } = renderHook(() => useCustomHook());

  // Assertions based on the hook's return value
  expect(result.current.someValue).toBe(/* expected value */);
});

[itsMapleLeaf]

Alright, that's very curious 🤔 The rule seems to just ignore unnamed callbacks passed to other functions

I don't know if I like this heuristic. Off the top of my head, I feel like it'd be more sensible to search specifically for functions named test, it, and describe and maybe allow configuring others.

[itsMapleLeaf]

Somewhat related to the above, they're apparently planning on letting you call use inside of useMemo

[arendjr]

I don't know if I like this heuristic. Off the top of my head, I feel like it'd be more sensible to search specifically for functions named test, it, and describe and maybe allow configuring others.

I'm a bit split on this one as well. I would appreciate it if we didn't need additional config for this; it doesn't feel intuitive if this required configuration. But I also agree the heuristic leaves something to be desired.

Meanwhile, I do think we catch hook usages inside anonymous functions that are inside other hooks or component functions. So maybe the heuristic is acceptable if it only applies to top-level anonymous functions?

[arendjr]

Alright, seeing how I don't see a better solution here, I think it's best if we align our behavior with the ESLint rule. That means we keep the current rules inside components (hooks can only be called from the top-level of the component, calls inside any nested functions are forbidden), while applying the same heuristics as ESLint outside components (hooks can only be called from named functions that are components or hooks based on their naming conventions, while calls from anonymous functions are always allowed).

This will however change our behavior a bit and trigger diagnostics where we didn't before. I think it's an acceptable change, since arguably it's a bug we didn't trigger diagnostics in those places before, but I think it's better to include this in a minor release rather than a patch.