eslint-plugin-react-hooks useEffect autofix of adding function names causes a lot of infinite loops

[joshunger]

I've read "My function value is constant" from #14920 (comment).

There is a problem on the opposite spectrum of this, which is where you get infinite loops (a function value always changes). We catch that in the lint rule now when possible (in the same component) and suggest a fix. But it's tricky if you pass something several levels down.

If you autofix the useEffect, would it also be possible to autofix any functions added by wrapping them in a useCallback at the same time?

This would greatly improve the user experience.

[gaearon]

A much better fix is usually to either move a function either outside of the component (if it doesn't use props or state), or move it inside the effect (if it does). useCallback is the escape hatch for when you can't or don't want to do either. So I don't think we'd want to autofix it to useCallback every time.

We could offer an option that disables the autofix when we detect function deps.

[joshunger]

That would be a better fix. I think a lot of people end up on your original code example #14326 (comment).

[joshunger]

You could leave autofix if there was a better solution for detecting infinite async loops.

[gaearon]

Thanks, I updated that example.

[RIP21]

@gaearon I'm sorry for the completely unrelated question in the topic (feel free to delete it)
But is the extraneous-deps rule is stable now to use, and all problems and false positives have been sorted?

[gaearon]

We've been using it at FB for a few weeks now and are pretty happy with it.

[dagda1]

I have a problem when --fix is enabled on this line. It changes the deps to:

  const ref = useRef<HTMLElement>(null);
  const [shortcuts, setShortcuts] = useState<ShortcutAction[]>();

  useLayoutEffect(() => {
    const trapper = scoped && ref.current ? new mousetrap(ref.current) : mousetrap;

    const map = mapKey ? (shortcutMap[mapKey] as Shortcut) : (shortcutMap as Shortcut);

    const shortcutActions = buildShortcuts(map);

    shortcutActions.forEach((shortcut) => {
      trapper.bind(shortcut.keys, (e: ExtendedKeyboardEvent) => {
        e.preventDefault();
        e.stopPropagation();

        handler(shortcut.action, e);
      });

      shortcut.trapper = trapper;
    });

    setShortcuts(shortcutActions);

    return () => {
      if (!shortcuts) {
        return;
      }

      shortcuts.forEach((shortcut) => {
        if (shortcut.trapper) {
          shortcut.trapper.unbind(shortcut.keys);
        }
      });
    };
  }, [handler, mapKey, scoped, shortcutMap, shortcuts]);

Adding the shortcuts dep will cause an infinite loop as setShortcuts is called which will cause a rerender.

[dagda1]

I have this shortterm fix:

const previousHandler = usePrevious(handler);

  useLayoutEffect(() => {
    if (shortcuts && handler === previousHandler) {
      return;
    }

I very well could be approaching this wrong :)

[amh4r]

Maybe I'm missing something, but how would I prevent infinite loops in this example?

CodePen example

By adding onChange() as a dep, changing the "Primary Input" goes into an infinite loop.

EDIT:

Solved it with useCallback(). Here's the CodePen.

[wcastand]

Hello, the eslint rule is creating an infinite loop in my case too:

gist

I'm guessing there is a better way to do what i do without the infinite loop and disabling eslint.

TLDR:
on my React app, i can receive the code for oauth2.
when i do, i make a fetch to my backend to get an access_token, then i register the token in localStorage.

If i have a token or the token change, i make a call to my backend to get a list of artists.

In both cases, i use in the useEffect: setArtists or setToken and the eslint rules add them to the array, which creates an infinite loop.

PS: the solution for @goodoldneon is not working in my case, when i copy his code into my editor, the autoformat is adding the setX into the array of useCallback and will create the same issue.

[amh4r]

@wcastand, is your editor saying you need the useState() set functions as deps in useCallback()?

[wcastand]

Yeah, i'm using vscode if that helps. The auto format/fixing is adding the setters and vars in the array and that creates an infinite loop.
I added //eslint-disable-next-line and now it works but that feels like a hack lol

[gaearon]

@wcastand The correct fix in your case is to wrap customSetState that’s returned from your custom Hook into useCallback. Then it can safely be specified as a dependency (like it should be).

Please read the documentation which explains all of these cases one by one:

• https://reactjs.org/docs/hooks-faq.html#is-it-safe-to-omit-functions-from-the-list-of-dependencies (your case is the third one in this section)

• https://reactjs.org/docs/hooks-faq.html#what-can-i-do-if-my-effect-dependencies-change-too-often

[wcastand]

Thanks for the help, sorry for the delayed answer, i'll try this out when i can but the project was based on spotify api and i am hitting limit rate for various reason and limitation of their API.

Anyway thanks for the example, this issues won't be issues anymore after Suspense and cache are introduced for React right?

[louisgv]

@gaearon I made a reproduction of a custom hook to work with array: https://codesandbox.io/s/hopeful-meadow-629zh

The set function is re-exported from the dipatcher of useState(), however eslint throw error as I'm calling it from the object. Wonder what is your thought as to whether this is a valid case?

My thinking is that I could eitehr useReducer, or export my hook as an array, or memo the object I'm exporting from my hook :-? ...

[gaearon]

Yeah you can memo that object.

[gaearon]

It would probably make sense to me if you didn’t keep using the object directly. But destructure individual functions from it instead. Then they can be more stable (for example a setter would never need to change). You could useCallback for those individual functions.

[gaearon]

Regarding the autofix, I hope eslint/rfcs#30 will let us solve the problem without compromising on IDE suggestions.

[gaearon]

Let's consolidate and track this in #16313 instead.

[gaearon]

This is resolved now.
#16313 (comment)