[ESLint] 'exhaustive-deps' lint rule - false warning and incorrect correction

[TriStarGod]

Do you want to request a feature or report a bug?
I have eslint autofix my errors. For the first time, it caused a bug rather than helped.
Eslint will auto add dependencies that cause the code to improperly function. In my example, it added onClick. Then it would request me to change onClick to a useCallback (FYI, I did not show the useCallback changes). If I did make those changes, it would disrupt the order the test buttons disappear.
What is the current behavior?

Current code
function TimeoutAlert({ id, message, deleteAlert, autoClose }) {
  const onClick = () => deleteAlert(id);
  useEffect(() => {
    if (autoClose) {
      const timer = setTimeout(onClick, 2000);
      return () => clearTimeout(timer);
    }
  }, [autoClose, onClick]);
  return (
    <p>
      <button onClick={onClick}>
        {message} {id}
      </button>
    </p>
  );
}

CodeSandbox link: https://codesandbox.io/s/multiple-alert-countdown-294lc

What is the expected behavior?

function TimeoutAlert({ id, message, deleteAlert, autoClose }) {
  const onClick = () => deleteAlert(id);
  useEffect(() => {
    if (autoClose) {
      const timer = setTimeout(onClick, 2000);
      return () => clearTimeout(timer);
    }
  }, [autoClose]);
  return (
    <p>
      <button onClick={onClick}>
        {message} {id}
      </button>
    </p>
  );
}

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
react 16.8.6
eslint-plugin-react-hooks 1.6.0

[TriStarGod]

I reworked it using refs. Is this the "proper" way?

function TimeoutAlert({ id, message, deleteAlert, autoClose }) {
  const savedCallback = useRef();
  const onClick = useCallback(() => deleteAlert(id), [deleteAlert, id]);
  useEffect(() => {
    savedCallback.current = onClick;
  }, [onClick]);
  useEffect(() => {
    if (autoClose) {
      const timer = setTimeout(savedCallback.current, 2000);
      return () => clearTimeout(timer);
    }
  }, [autoClose]);
  return (
    <p>
      <button onClick={onClick}>
        {message} {id}
      </button>
    </p>
  );
}

https://codesandbox.io/s/multiple-alert-countdown-7z2d4

[TriStarGod]

Also, if I create a custom hook, it no longer warns me about memoizing the onClick function. Is that a bug or is that on purpose... I don't see the purpose of memoizing the callback for the current or previous code sample since it only runs once.

function useTimeout(callback, ms) {
  const savedCallBack = useRef();
  // Remember the latest callback
  useEffect(() => {
    savedCallBack.current = callback;
  }, [callback]);
  // Set up timeout
  useEffect(() => {
    if (ms !== 0) {
      const timer = setTimeout(savedCallBack.current, ms);
      return () => clearTimeout(timer);
    }
  }, [ms]);
}

https://codesandbox.io/s/multiple-alert-countdown-973b3

[gaearon]

I have eslint autofix my errors.

Yeah.. That's not how this rule is supposed to be used. I wish there was a way to mark ESLint autofix suggestions as manual — so that you can still autofill them from an IDE but they would require a manual review. For this rule, it's supposed that you always read the message.

See #15204 (comment) for past discussions. The rule points out correct problems, but the fix isn't always what it suggests (because sometimes you need to restructure your code). I think it's possible we'll hide autofix behind an option in the future. Or, ideally, work with ESLint to add a way to flag autofix suggestions as IDE-only.

This might help understand it in the meantime:

• https://reactjs.org/docs/hooks-faq.html#what-can-i-do-if-my-effect-dependencies-change-too-often
• https://reactjs.org/docs/hooks-faq.html#is-it-safe-to-omit-functions-from-the-list-of-dependencies

[gaearon]

Here's a fixed version: https://codesandbox.io/s/multiple-alert-countdown-xrfwq. Since the function is passed down and is used in an effect, I wrap it in useCallback. That lets me ensure its identity is stable for as long as necessary.

[gaearon]

The reason the lint rule complaints is because deleteAlert could be using a prop or state. So using a version from the first render would be wrong in that case.

Also, if I create a custom hook, it no longer warns me about memoizing the onClick function. Is that a bug or is that on purpose.

It's on purpose because you're not breaking the rules anymore. With your solution you always get the latest onClick which references the latest deleteAlert. That's probably what you want. The ref solution is reasonable for many of these cases. (It's also essentially what a class solution would do.)

One downside is that the ref solution wouldn't let your effect "react" to changes in deleteAlert. For example if deleteAlert used some prop, and that prop changed, would you want the effect to be cleaned up and re-created? If yes, you'll want to make it a dependency like I did in #16001 (comment). If no, you'll want to keep it as a ref.

I'll close this issue since core complaint is same as #15204. Hope this helps. I hope FAQ resources above are also helpful. (And if you want a real deep dive, I wrote about it here and here.)

[TriStarGod]

I really appreciate the response. I considered useCallback and useMemo as performance optimizations for computationally expensive items rather than referencing functions / variables. I'll reevaluate my mindset for React hooks.

[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.

[gaearon]

This is resolved now.
#16313 (comment)