react icon indicating copy to clipboard operation
react copied to clipboard

Bug: eslint-plugin-react-hooks treats a non-react `use` function the same way as `React.use`

Open artaommahe opened this issue 1 year ago • 5 comments

React version: 18.3.1 eslint-plugin-react-hooks: 5.0.0

Steps To Reproduce

  1. install eslint-plugin-react-hooks 5.0.0
  2. write this code
function smth(use: () => void) {
  use();
}
  1. get an error React Hook "use" is called in function "smth" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use".eslint[react-hooks/rules-of-hooks](https://reactjs.org/docs/hooks-rules.html)

We got this error with use function in the callback of playwright's test.extend()

const test = base.extend<{ track: E2EGeneratedTrackFragment }>({
  track: async ({}, use) => {
    // --------------^ local `use` function
    const track = await generateTestTrack('e2e');
    await addTrackToArcade(track.id);
    await use(track);
    // -----^ the error is here
    await deleteTestTrack(track.id);
  },
});

Link to code example: -

The current behavior

any custom use function is treated like React.use

The expected behavior

only use() function from react is linted by those rules

Additional

There is no error with eslint-plugin-react-hooks v4.6.2

artaommahe avatar Oct 14 '24 08:10 artaommahe

Is this documented by Playwright to be named use?

eps1lon avatar Oct 14 '24 13:10 eps1lon

yes

export type TestFixture<R, Args extends KeyValue> = (args: Args, use: (r: R) => Promise<void>, testInfo: TestInfo) => any;

also every example on this page https://playwright.dev/docs/test-fixtures#with-fixtures

And it's not just about playwright usage, it's more about preventing developers from using use as a custom function name anywhere in the react codebase with this rule enabled

artaommahe avatar Oct 14 '24 13:10 artaommahe

My grandpa is here blustering at me saying "it's kind of amazing that the decision to name a hook use made it through review"

He sayin "Nobody thought about how the ESLint enforcement would effectively hijack an entire verb?" "No rule config to enable exceptions?"

I told him "Peepaw, just write your own ESLint rule? You don't have to use use"

(At that point, we looked at each other in unspoken agreement that having to now say 'use use' is a deeply unfortunate state of affairs)

And then he got quite loudly (when he gets loudly it gets very loud indeed) Lot of old timey swears and something about how he "doesn't have the time to fiddle with ESLint rules and nested glob patterns for the rest of his life"

He's a bit of a firecracker. I'm trying to reason with him because of course I can see both sides. But I thought I'd share since it's interesting feedback.

leggomuhgreggo avatar Nov 16 '24 09:11 leggomuhgreggo

What if the rule logic were enhanced such that — when used as a an object method — it will only apply when the object is React?

Something like:

Applies

React.use()

Does not apply

AnyOtherObj.use()

leggomuhgreggo avatar Nov 18 '24 20:11 leggomuhgreggo

What if the rule logic were enhanced such that — when used as a an object method — it will only apply when the object is React?

Something like:

Applies

React.use()

Does not apply

AnyOtherObj.use()

In fact, it is quite possible to determine whether that use is from React at static analysis level, and not only when used as a an object method. I've created a debug rule to prove it works. Xnip2024-11-22_21-37-46

For anyone interested in the implementation just check out this file.

Probably 60 lines of code implemented using the common ESLint techniques will suffice for most scenarios, without the need for ts or flow type information.

Rel1cx avatar Nov 22 '24 13:11 Rel1cx

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

github-actions[bot] avatar Feb 20 '25 14:02 github-actions[bot]

bump

artaommahe avatar Feb 20 '25 16:02 artaommahe

I was curious how complex the code was that @Rel1cx shared and it goes deep, with many helper functions. Feel free to correct me if it's not as bad as it looks. Not sure if people are fine with having all that additional code, or if there's a simpler, more concise way.

mikedidomizio avatar Feb 24 '25 13:02 mikedidomizio

bump

imdadul avatar Feb 25 '25 13:02 imdadul

I was curious how complex the code was that @Rel1cx shared and it goes deep, with many helper functions. Feel free to correct me if it's not as bad as it looks. Not sure if people are fine with having all that additional code, or if there's a simpler, more concise way.

It's not that complex as it seems. The reason there are so many helper functions behind the implementation of that shared rule is that (even half year before this issue was created) this has already been an undocumented configurable feature in eslint-react, supported by all its rules to detect all React APIs. Here, we only need to detect a use hook, and even if other hooks are included, the situation would still be much simpler.

Rel1cx avatar Feb 26 '25 13:02 Rel1cx

For now, I ended up excluding this rule for playwright files:

// eslint.config.mjs
export default [
  // ...
  {
    files: ["playwright/**/*.{js,mjs,cjs,ts,jsx,tsx}"],
    rules: {
      "react-hooks/rules-of-hooks": "off"
    }
  }
];

rezam7596 avatar Mar 07 '25 13:03 rezam7596

Can I have a code example and insight into how it detects when the rule is broken so we can fix the issue?

moshe15920 avatar Mar 27 '25 08:03 moshe15920

After investigating the issues raised here and in #15227, it's clear that the current Rules of Hooks behavior - flagging any function starting with use* - can lead to false positives in non-React contexts (e.g. Playwright's use(), utility libraries like useWith()).

A potential solution could be (and which I tried to implement) to only treat functions as hooks if they're:

  • Imported directly from React or known hook libraries.
  • Defined locally and call other recognized hooks (including transitive usage).

However, without cross-file analysis, this approach would miss violations in custom hooks that are imported from other files and internally use hooks like useState() or useEffect(). In that scenario, the linter would treat them as regular functions and fail to enforce the rules, which significantly weakens the reliability of the rule.

Given these tradeoffs, the current strict behavior - while occasionally overreaching - is a safer default. See also the comment from @gaearon: Yeah it's intentional....

It ensures consistent enforcement across projects, especially in large codebases with shared custom hooks and limited static context. For now, maintaining the current implementation seems like the most reliable strategy. Future improvements might explore opt-out mechanisms (e.g. @noHook annotations or scoped overrides), but any change must preserve the rule's core purpose: preventing invalid hook usage with a high degree of confidence.

oyershov avatar Apr 03 '25 07:04 oyershov

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

github-actions[bot] avatar Jul 15 '25 08:07 github-actions[bot]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

github-actions[bot] avatar Jul 22 '25 09:07 github-actions[bot]