useHookAtTopLevel reports errors on code that is not React

[ssilve1989]

Environment information

CLI:
  Version:                      1.8.3
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v20.11.1"
  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:                 false

Workspace:
  Open Documents:               0

What happened?

The following sample NestJS snippet triggers the useHookAtTopLevel thinking useGlobalInterceptors is a React hook.

async function bootstrap() {
  const app = await NestFactory.create(AppModule)
  app.useGlobalInterceptors(new RandomInterceptor());
}

bootstrap();

Expected result

It should not raise any warning/error

Code of Conduct

• I agree to follow Biome's Code of Conduct

[yossydev]

Reproduced here. It is indeed an error.
https://github.com/yossydev/use-hook-at-top-level-nextjs

I'm not familiar with nestjs and don't know what use you are using useHookAtTopLevel for, but I would have thought it would be better not to check lint as this is a React rule in the first place.

[dyc3]

That link 404s.

This rule should only be checked in jsx or tsx environments. I don't think we currently have a way to automatically determine if code is react code or not other than that.

[yossydev]

That link 404s.

Sorry, this was private repository.
you can see it now.

This rule should only be checked in jsx or tsx environments. I don't think we currently have a way to automatically determine if code is react code or not other than that.

Certainly, that sounds better.

[Javimtib92]

I think we could use a couple of heuristics like checking is_react_component (checks if function starts with Uppercase like react components do) and checking if the file is jsx/tsx.

Could I take this task to try that out?

[dyc3]

Sure! I'll go ahead and assign you.

[arendjr]

Please be aware we already have such a heuristic:

biome/crates/biome_js_analyze/src/react/hooks.rs

Line 132 in d3dacc9

pub(crate) fn is_react_component(name: &str) -> bool {

I suppose this is not used by the rule yet…

[Javimtib92]

Please be aware we already have such a heuristic:

biome/crates/biome_js_analyze/src/react/hooks.rs

Line 132 in d3dacc9

pub(crate) fn is_react_component(name: &str) -> bool {

I suppose this is not used by the rule yet…

Yep, that's the function I wanted to use. I was thinking now that maybe limiting this to jsx or tsx files is not a good heuristic because a hook could be used inside another hook defined in a js or ts file, right?

For example: https://github.com/ilyalesik/react-media-hook/blob/master/index.js

Edited: Thinking about it a bit more, only checking if it's inside a react component is not correct either because it might be used inside another hook. I'll take that into consideration too.

[Javimtib92]

Investigating this I noticed that eslint approach is not perfect either:

[Javimtib92]

A similar case was fixed for some jest functions that start with “use”

biome/crates/biome_js_analyze/src/react/hooks.rs

Line 156 in d3dacc9

if let Some(expr) = call

I tried adding other heuristics, like checking that the root caller is a react component or a hook, but so far all I tried ends up changing the snapshots because some cases get uncovered. We might need to do some comprimises with this.

[dyc3]

It might be useful if biome could inspect package.json to guess what environment the file is in.

[Javimtib92]

@dyc3 I think your suggestion is actually a good workaround. I opened a Draft PR with this change. I need to find a way to run the snapshots simulating an environment with the react dependency. I would highly appreciate any suggestions. Thanks!

[Conaclos]

There is something I don't understand: the rule is not enabled by default (it is not recommended). Thus, the rule should only be enabled if the project use React. Why did you enable it in the first place?

[ematipico]

Maybe the documentation could be improved, but this rule isn't meant for non-react projects, so I don't think it's a bug on our side.

[dyc3]

I agree with @ematipico I'm all for saving user's time configuring projects, and maybe we could come up with some really good heuristics to auto detect what kind of project biome is operating on, but heuristics are not perfect. If anything, it would probably be better to ask the user for that info in biome init when they are setting it up.

Would it make sense to have more collections of rules similar to how we currently have recommended rules? eg. react-recommended, react-all. Or we could give rules tags to do something similar, though tags might be a bit more complicated on the implementation side.

[ematipico]

Would it make sense to have more collections of rules similar to how we currently have recommended rules?

There's an RFC for that, you can find it from the pinned umbrella issue "Biome developments"

[ssilve1989]

There is something I don't understand: the rule is not enabled by default (it is not recommended). Thus, the rule should only be enabled if the project use React. Why did you enable it in the first place?

@Conaclos because I am an idiot apparently. I had all rules enabled and then after adding some offending code one day saw this error start appearing and didn't remember I had all instead of recommended