[eslint-plugin-react-hooks] allow configuring custom hooks as "static"

[grncdr]

Do you want to request a feature or report a bug?

Feature/enhancement

What is the current behavior?

Currently the eslint plugin is unable to understand when the return value of a custom hook is static.

Example:

import React from 'react'

function useToggle(init = false) {
  const [state, setState] = React.useState(init)
  const toggleState = React.useCallback(() => { setState(v => !v) }, [])
  return [state, toggleState]
}

function MyComponent({someProp}) {
  const [enabled, toggleEnabled] = useToggle()

  const handler = React.useCallback(() => {
    toggleEnabled()
    doSomethingWithTheProp(someProp)
  }, [someProp]) // exhaustive-deps warning for toggleEnabled

  return <button onClick={handler}>Do something</button>
}

What is the expected behavior?

I would like to configure eslint-plugin-react-hooks to tell it that toggleEnabled is static and doesn't need to be included in a dependency array. This isn't a huge deal but more of an ergonomic papercut that discourages writing/using custom hooks.

As for how/where to configure it, I would be happy to add something like this to my .eslintrc:

{
  "staticHooks": {
    "useToggle": [false, true],  // first return value is not stable, second is
    "useForm": true,             // entire return value is stable 
  }
}

Then the plugin could have an additional check after these 2 checks that tests for custom names.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

All versions of eslint-plugin-react-hooks have the same deficiency.

Please read my first comment below and try my fork if you are interested in this feature!

[grncdr]

I went ahead and implemented this to see how it would play out in my own codebase. If anybody else feels like trying it, I've published it to npm under @grncdr/eslint-plugin-react-hooks.

To try out my fork:

Install it

Update your package.json to install @grncdr/eslint-plugin-react-hooks:

-    "eslint-plugin-react-hooks": "...",
+    "@grncdr/eslint-plugin-react-hooks": "5.0.0-p30d423311.0"

Update your .eslintrc

You will need to update your eslintrc to reference the scoped plugin name and configure your static hook names:

-  "plugins": ["react-hooks"],
+  "plugins": ["@grncdr/react-hooks"],
   "rules": {
-    "react-hooks/rules-of-hooks": "error",
-    "react-hooks/exhaustive-deps": "warn",
+    "@grncdr/react-hooks/rules-of-hooks": "error",
+    "@grncdr/react-hooks/exhaustive-deps": [
+      "error",
+      {
+        "additionalHooks": "usePromise",
+        "staticHooks": {
+          "useForm": true,
+          "useRouter": true,
+          "useEntityCache": true,
+          "useItem": [false, true],
+          "useQueryParam": [false, true],
+          "useSomeQuery": {
+            "reload": true,
+            "data": false,
+            "error": false,
+            "isLoading": false
+          }
+        }
+      }
+    ],

(note the hook names above are specific to my app, you probably want your own)

The staticHooks config maps a hook name to the "shape" of return values that are static, as described above in the original issue. Given the above examples...

"useRouter": true means the return value of useRouter is considered stable.

"useQueryParam": [false, true] this defines a useState-like hook that returns an array of [value, setQueryParam]. The value is not stable, but the setter is.

"useSomeQuery": { ... }" this defines a react-query-like hook that returns a complex object. That object includes a reload callback that is stable, but the data/error/isLoading properties are not.

---

If anybody from the React team thinks the idea is worth pursuing I'll try to add some tests and make a proper PR.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[VanTanev]

This seems like a really great addition, would love to see it in react-hooks

[grncdr]

@VanTanev have you tried my fork? I've been using it since my last comment and haven't had any issues, but positive experience from others would presumably be interesting to the maintainers.

[ramiel]

Any news on this. It's very annoying now because you cannot use reliably this lint rule when you use custom hook, so you have to disable the rule leading to potential dangerous situations

[kravets-levko]

IMHO it would be great if this plugin could detect some common "static" patterns in custom hook, for example if custom hook returns result of useRef()/useCallback(..., [])/useMemo(..., []) etc.

[ramiel]

Indeed. Still there may be ambiguous situations and so having the ability to set it up through options could still be needed

[AriTheElk]

Commenting to bump this thread and show my interest. Working on a large codebase with lots of custom hooks means that this would allow us to more reliably use the hooks linter. I understand that the reason they might not want to allow this, is because it could enable people to introduce dangerous bugs into their apps. So maybe it's a feature that should be added with a large disclaimer.

It's pretty likely that the maintainers simply don't want to deal with bug reports that are related to people setting their hooks to static when they actually aren't static. A lot of people will misunderstand what it means to have static hooks.

[grncdr]

IMHO it would be great if this plugin could detect some common "static" patterns in custom hook, for example if custom hook returns result of useRef()/useCallback(..., [])/useMemo(..., []) etc.

This is way beyond the scope of what ESLint can do (it would require whole-program taint-tracking) so definitely not going to happen here.

I understand that the reason they might not want to allow this, is because it could enable people to introduce dangerous bugs into their apps. So maybe it's a feature that should be added with a large disclaimer.

It's pretty likely that the maintainers simply don't want to deal with bug reports that are related to people setting their hooks to static when they actually aren't static. A lot of people will misunderstand what it means to have static hooks.

I would totally understand this point of view, but until somebody from the React team replies, I'll keep hoping (and using my fork 😉).

[ksjogo]

@grncdr can you please point me to the source of your folk?

[grncdr]

@ksjogo sure, my changes are in this branch: https://github.com/grncdr/react/tree/eslint-plugin-react-hooks-static-hooks-config

You can use it by installing @grncdr/eslint-plugin-react-hooks and updating the config as described in https://github.com/facebook/react/issues/16873#issuecomment-536346885

[douglasjunior]

This is really missing for us, because we have hooks like useAxios that always return the same value.

We have faced problems such as:

const axios = useAxios(...);

const requestSomething = useCallback(() => {
      return axios.get(...);
}, []);

ESLint warning:

React Hook useCallback has a missing dependency: 'axios'. Either include it or remove the dependency array.eslint(react-hooks/exhaustive-deps)

[grncdr]

I’m curious about that use case: what is the useAxios hook doing that couldn’t be accomplished with a normal import?

[douglasjunior]

I’m curious about that use case: what is the useAxios hook doing that couldn’t be accomplished with a normal import?

Internally it uses useMemo to create an instance of axios, and also a useEffect that cancels pending requests when the component is unmounted.

Additionally, it configures the baseUrl and automatically injects the authentication token via interceptor.

[acnebs]

I would also like to see this behavior, mostly just for setState and useRef.

@douglasjunior don't want to get too off-topic, but you might just wanna have a global/singleton/etc. for that? Seems unnecessary to set the baseUrl and token every time you use the hook, as presumably those values will not change between instances of the hook.

[douglasjunior]

@douglasjunior don't want to get too off-topic, but you might just wanna have a global/singleton/etc. for that? Seems unnecessary to set the baseUrl and token every time you use the hook, as presumably those values will not change between instances of the hook.

The useAxios is configurable, it can receive a custom baseURL, and others configs.

But in the end it makes no difference, the main purpose for us is to cancel pending requests, and make the axios instance private to the component.

[squirly]

Allowing configuration of the dependency argument position would be useful as well.

It is currently hard coded to 0 for additionalHooks: https://github.com/facebook/react/blob/8b580a89d6dbbde8a3ed69475899addef1751116/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js#L1361

This allows support for hooks that take more than 2 arguments. Eg.:

useImperativeHandle(ref, callback, deps)

I've separately implemented something along the lines of:

rules:
  customHookDeps:
    - error
    - additionalHooks
      - useEventListener: 1
      - useCustomHook: 0
      - useOtherHook

Where the regex syntax can still be supported.

[quicksnap]

Food for thought: if ESLint is able to leverage any TypeScript information, there could be a way to type-level annotate hooks accordingly.

[grncdr]

I think this discussion would benefit from some clarification of what is possible and what is feasible. To that end, I'm writing below the limits on what I would personally propose. I certainly don't know everything about what can be done with ESLint, so if you read this and think "he doesn't know what he's talking about" please correct me!

Limits of this proposal

Couldn't we infer this automatically?

Not using ESLint. Or alternatively, not without making this ESLint plugin extremely complicated. Even if somebody did that work there's no indication the React team at Facebook would want to maintain it.

Could we annotate the "staticness" of a hook in the source code? (using types and/or comments)

Unfortunately no, the reason is that the ESLint plugin must analyze the locations a variable is used and not where it's declared. At minimum, you would need to annotate a hook every time you import it, since ESLint works on a file-by-file basis.

Could a type checker do this automatically?

After reading the above you might think that Typescript or Flow could be leveraged to tell us when a return value is static. After all, they have the global information about values in separate modules that a linter doesn't.

However, neither of them (as far as I'm aware) let you talk about the type of the implicit environment created by a closure. That is, you can't refer to the variables captured by a function. Without this, you can't propagate information about the closed-over variables to the return type. (If the type systems did have this capability, you theoretically wouldn't need to write the dependency arrays at all)

--

[douglasjunior]

I think it is possible to pass a parameter to "eslint-plugin-react-hooks" in .eslintrc, with the names of the hooks that are static?

Something like what we do with globals?

Sorry if I'm wrong.

[grncdr]

I think it is possible to pass a parameter to "eslint-plugin-react-hooks" in .eslintrc, with the names of the hooks that are static?

Yep, that’s what this issue proposes and what I’ve implemented (see my earlier comments for details). I just wanted to clarify that I think the explicit configuration makes the best possible trade off in terms of implementation complexity.

[quicksnap]

I think it would be great to have this. Anyone know how to get feedback from a maintainer to see if we can move forward with this?

[douglasjunior]

Suggestion: Follow the same idea as the "camelcase" rule and add "static" option.

https://github.com/eslint/eslint/pull/10783

[grncdr]

@douglasjunior could you provide an example of what you mean? I didn’t understand what you wanted to demonstrate with the PR you linked.

[alirezamirian]

I'm a little late to the party but I think a better approach would be to infer such cases by tracing the retuned values and see if it's something static. If this is not feasible, or doesn't make sense from a performance point of view, maybe we can at least annotate each custom hook to provide such information in a jsdoc comment block like this:

/**
 * Inspired from the format that is suggested in the discussions above:
 * @react-hook: [false, true]
 */
function useToggle(init = false) {
  const [state, setState] = React.useState(init);
  const toggleState = React.useCallback(() => {
    setState(v => !v);
  }, []);
  return [state, toggleState];
}

Advantages of this approach:

• The information about static parts of the return value is encapsulated in the code. So a third-party library can ship this information with the code without needing the user to adjust their es-lint configuration to make it work.
• It's still fairly easy to capture this information from the AST without advanced and heavy AST processing.

Disadvantages of this approach:

In the meanwhile that third-party libraries adopt this feature, there is no way to teach eslint-plugin-react-hooks about such static stuff. i.e. the same advantage of being able to put this information in the code can become a disadvantage when you don't have access to the code and it doesn't include this information for any reason.

[grncdr]

@alirezamirian do you know if ESlint makes it possible/easy to get the AST information for imported modules? I was under the impression it only worked on a single file at a time.

[alirezamirian]

@grncdr That's a good point. I'm not an eslint expert but I think you are right and we only have Access to Program node corresponding to a single file. The best we can get from the AST in hand is the import statement source. I don't know if there is a utility for resolving AST from an import declaration.

UPDATE: There is a parserPath property on the context, which has a parseForEslint function which can be used to parse other files. So it's technically feasible. But I'm not sure if it's a right approach and it's meant to be used like this.

[stale[bot]]

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!

[kravets-levko]

bump

[quicksnap]

Just another "+1" post, but I'd like to add in that, while there are workarounds, such as using refs or hooks to wrap that kind of logic, it feels unnecessarily boilerplate-y. Having a pragma for ignored values would be so valuable--often devs lazily and dangerously just turn off the whole block and loose safety.

[stale[bot]]

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!

[Donov4n]

Bump ...

[masad-frost]

Sent a PR that marks hooks as stable https://github.com/facebook/react/pull/20513, though it doesn't do it for destructutred array types which is proposed here.

I think this is a good first step and the team would more likely to accept something simple.

[grncdr]

FYI for those still following this issue. I've updated rebased my fork on top of the latest release (v4.2.0) and published a new version @grncdr/eslint-plugin-react-hooks@4.2.0-fix. It still works great for me.

[grncdr]

Allowing configuration of the dependency argument position would be useful as well.

It is currently hard coded to 0 for additionalHooks:

https://github.com/facebook/react/blob/8b580a89d6dbbde8a3ed69475899addef1751116/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js#L1361

This allows support for hooks that take more than 2 arguments. Eg.:

useImperativeHandle(ref, callback, deps)

I've separately implemented something along the lines of:

rules:
  customHookDeps:
    - error
    - additionalHooks
      - useEventListener: 1
      - useCustomHook: 0
      - useOtherHook

Where the regex syntax can still be supported.

I actually needed this for the first time yesterday, so I implemented it in my fork. Available now as @grncdr/eslint-plugin-react-hooks@4.2.0-fix2

[adrianhelvik]

I'd love to be able to add an annotation when importing the hook like this:

import /** @react/constant-hook */ useClosure from './useClosure'

I would find this pretty useful for implementing stuff dealing with event handling so forth. Defining it when exporting would be even better, but I'd guess that is out of scope for Eslint.

import { useCallback, useRef } from 'react'

/**
 * @react/constant-hook
 */
export default function useClosure(fn) {
  const ref = useRef()
  ref.current = fn

  return useCallback((...args) => {
    return ref.current(...args)
  }, [])
}

[grncdr]

Defining it when exporting would be even better, but I'd guess that is out of scope for Eslint.

Yes, at least as far as my ESLint skills go.

---

I'm curious, are you saying you are only interested in solutions where you can annotate the static-ness in your source code? (instead of the ESLint config)

If you are ok with defining it in the ESLint config you would just need to switch to my fork and add this to your config:

    "@grncdr/react-hooks/exhaustive-deps": [
      "error",
      {
        "staticHooks": {
          "useClosure": true
        }
      }

[kravets-levko]

I'm curious, are you saying you are only interested in solutions where you can annotate the static-ness in your source code? (instead of the ESLint config)

Just my .02: with a config it's very easy to get code and config out of sync, especially if the team is big. Adding annotations to imports is also not great - if you update your hook you'll have to update all imports accordingly. Ideally the hook itself and its annotations should be in the same file (just as JSDocs for example).

P.S. What if I'll have multiple useClosure hooks in different folders, and they all have different returning values?

[grncdr]

P.S. What if I'll have multiple useClosure hooks in different folders, and they all have different returning values?

This problem already exists with the current eslint plugin:

import {useEffect, useState} from 'react'

const useMyState = useState

export function FooComponent() {
  const [x, setX] = useMyState(1)
  useEffect(() => setX(x => x + 1), []) // exhaustive hook deps error, missing dependency `setX`
  return <span>X is {x}</span>
}

Ideally the hook itself and its annotations should be in the same file (just as JSDocs for example).

I agree, but as explained multiple times in this thread this isn't possible as long as ESLint works on one file at a time. As before, I'd love it if somebody showed up and proved me wrong.

[conradreuter]

I agree, but as explained multiple times in this thread this isn't possible as long as ESLint works on one file at a time. As before, I'd love it if somebody showed up and proved me wrong.

For the more real-world cases I can think of, I believe ESLint could attempt to check the import source instead of just the name. I.e. the following should be doable:

import {useState as useWhatever} from 'react'

export function Component() {
  const [x, setX] = useWhatever(1)
  // expecting no error, because it is the second value returned by`useState` imported from `'react'`
  useEffect(() => setX(x => x + 1), [])
  return <span>{x}</span>
}

For this to work, the configuration of the ESLint plugin would contain both the filename and the export name.

If that turns out to be doable, then the following should be doable as well, given an appropriate configuration:

import {useStaticValue} from '../hooks/static-value'

export function Component() {
  const x = useStaticValue()
  // expecting no error, because `useStaticValue` from `path/to/hooks/static-value` is configured to be static
  useEffect(() => doSomething(x), [])
  return <span>{x}</span>
}

There will always be edge-cases, yet I think the import-source-and-name approach could be reasonably easy to implement, and cover a lot of real-world use-cases.

[grncdr]

@conradreuter that doesn't actually help with the criticisms from @kravets-levko being discussed. Which were:

Just my .02: with a config it's very easy to get code and config out of sync, especially if the team is big.

and

What if I'll have multiple useClosure hooks in different folders, and they all have different returning values?

The first criticism is valid: the ergonomics of external configuration is inferior to annotations on the definition of a hook. The reasons that annotations aren't going to work have (and had already) been covered multiple times in this thread. That's what I was referring to when I said "I'd love it if somebody showed up and proved me wrong".

The second criticism is less valid, because the ESLint plugin is entirely based around local names, and so far this has not been a problem in practice. That was my only reason for bringing up the renaming of hooks: I don't think it's actually a common practice in the real world. I never do it myself, so I'm not going to expend any effort to support it.

A more important point: it's now been 2 years and nobody from the React team has ever commented on or acknowledged this issue. I'll maintain my fork for as long as I'm working on React projects, but I think at this point there's no hope that this will be upstreamed.

[stale[bot]]

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!

[Donov4n]

Bump … 🙂

[ValentinH]

I think having such an option would be a great addition to the ecosystem. For example, for Next.js and its useRouter() hook. It is not referentially stable but should not be part of the dependency array. In fact, the Next.js documentation always omits it from the deps array:

This brings confusion to a lot of developers as in this discussion.

As a result, this rule is often set as a warning level, like in the official Next eslint config. Having the option to list some hooks whose results can/should be omitted from the dependency array would enable to improve these configuration. For example, Next configuration could whitelist by default the useRouter.

I've seen so many mistakes being done with effect dependencies by developers who are just used to quickly drop a // eslint-disable-next-line react-hooks/exhaustive-deps. I think that being able to whitelist the actual valid use cases would reduce this habit effect and be able to teach "you must follow this rule".

cc @gaearon

[scottrippey]

I strongly agree that this config is necessary. I maintain many libraries with custom hooks, and would absolutely benefit from this feature.

Just one example: https://github.com/scottrippey/next-current-route has 2 hooks that follow the useState pattern:

import { useRouteState, useCurrentRoute } from 'next-current-route';
//...
const [ id, setId ] = useRouteState('id');
//          ^^^^^ stable
const [ query, route ] = useCurrentRoute();
//             ^^^^^ stable

Also, this user-land implementation of useEvent which returns a stable reference, like useRef

const handler = useEvent(() => ...);
//    ^^^^^^^ stable

[gdbd]

is any chance of implement this? thread active from 2019 and still no solution

[grncdr]

is any chance of implement this? thread active from 2019 and still no solution

@gdbd did you try my fork? It's not an "official" solution but it works. See https://github.com/facebook/react/issues/16873#issuecomment-536346885 for instructions.

[gdbd]

@grncdr, yet not, at work i am cannot use forks

[OrkhanAlikhanov]

3 years almost but no progress on such a simple but useful addition. For those not wanting to use a fork, we can utilize patch-package to maintain our own patch locally. I created a patch out of @grncdr's work in his fork.

[Tsourdox]

This would be a great addition for all the written custom hooks.