react: [react-hooks/exhaustive-deps] eslint --fix breaks the code

@infodusha I understand how eslint-disable works. Problem is in semantic of eslint --fix.

Just imagine situation I have my version of code - it works fine, I pushed my changes to remote. CI executes ‘eslint --fix’ and deploy broken version of code to production (just because this is autofixable warning)

Eslint --fix should never change logic of your code. But in my example eslint with this rule will change logic of my code. I pretty sure that this warning should be not-autofixable with --fix.

@EugeneDraitsev // eslint-disable-line react-hooks/exhaustive-deps will help eslint --fix ignore this rule on line

@behzad888 I understand that my code is not correct from ‘hooks best practice’ point of view, but this code is still valid and will behave ‘as designed’. Often peoples will create hooks, that will be not follow best practices or even documentation.

As I said above and as @gaearon mentioned in 2nd part of his comment:

eslint --fix shouldn’t break the code (even if the code is already broken)

From my point of view make sense to change level of react-hooks/exhaustive-deps from warning to error and make it non-autofixable to prevent possibility to break user code. (or at least disable autofix by default)

Of Course important to add some “suggestions” for user (and IDE), but maybe just an error description will be enough in this case.

@gaearon - I arrived to this thread and similar ones, simply because eslint autofix for react-hooks/exhaustive-deps rule messed up my code. I believe many others have stumbled within various threads surrounding the same subject.

I’ve read threads on eslint regarding support for autofix disable for specific rules - Seems like there are no current solutions except some hacks you can do in your config + CI.

The eslint guys emphasized several times - it is highly recommended that autofix should not alter runtime behavior, let alone break code.

On one hand, we have a very important rule that we want to enforce. On the other, eslint autofix runs as part of our pre-commit hook which hides the fact that such drastic changes might have happened to your code.

This autofix behavior leaves us with two non-optimal options:

1. Disable the rule and rely on our devs to write their code in a correct way
2. Enable the rule and rely on our devs to check each and every relevant hook (useEffect, useMemo, etc.) in their PR, retest the code and make sure that it still runs as they’ve tested it before the commit.

I would rather have autofix for this completely disabled by default then being forced to choose one of the two options above. I believe most people will agree, based on the nature of this autofix.

Is there any work being done on configuring autofix for this rule? I haven’t seen any issue on this. Should I open one?

Your code is buggy. Hook must be like that:

import { useEffect, useState } from 'react'

export function useSwitchTab(trigger, tabsAmount, initialState = 0) {
  const [currentTab, setTab] = useState(initialState - 1)

  useEffect(() => {
    setTab(tab => tab + 1 >= tabsAmount ? 0 : tab + 1)
  }, [tabsAmount, trigger]) // eslint-disable-line react-hooks/exhaustive-deps
// also, trigger is unnecessary dependency by default so you need comment line in the top

  return [currentTab, setTab]
}

Had a similar issue: “The ‘xxx’ function makes the dependencies of useEffect Hook change on every render. To fix this, wrap the ‘xxx’ definition into its own useCallback() Hook”

The autofix then proceeded to wrap my function into useCallback()… without importing it, breaking my app.

Autofix should really be disabled by default.

@infodusha , Thanks for reasonable code fix. Main problem here, that eslint --fix will break my version code at all. Eslint shouldn’t break code in any case (it shouldn’t create endless loops in any situation)

The warning is not a false positive.

You can see how to fix it here:

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

In this particular case, the fix would be to function form of setState, like in the above comment.

It’s fair that there is an expectation that eslint --fix shouldn’t break the code (even if the code is already broken). The root of the issue here is that ESLint offers no way to offer “suggestions”. We’d like a way to indicate that the autofix isn’t guaranteed to work, but is a good starting point for the fix (e.g. for IDE integration that lets you manually apply it by clicking a suggestion icon).

We could disable autofix by default, and add an option that enables it. I don’t know if this is what most people want.

Yes, this seriously needs to be addressed. I just added this lint rule to a large codebase and ran autofix to resolve some of the basic issues. My assumption was that the logic would not be changed, as is the reasonable expectation with ESLint fixes. However, after testing my code I discovered that it had been broken in tons of components.

I get that there were technical issues in how the hooks were used, to begin with, which is exactly why there should have just been warning or error raised so that I had the opportunity to address them with the proper scrutiny. But having a potentially breaking fix is unreasonable and renders (what is otherwise a really good linting rule) rule dangerous.

Please please please disable the autofix by default, or remove it entirely. In the meantime, I forsee this causing breaking changes in our codebases as logic is automagically changed during pre-commit hooks.

I have another case to add to this question.

Suppose I have a hook like this:

function useWindowPosition() {
  const [position, setPosition] = useState({ left: 0, top: 0 });
  return [position, setPosition];
}

If I use it in this way (is a simple example so excuse me about the logic, but suppose the useWindowPosition hook returns as the second element of the array a complex function):

  const [position, setPosition] = useWindowPosition();
  useEffect(() => {
    setPosition({ left: 1, top: 1 });
  }, [userId]);

Eslint fires the following:

React Hook useEffect has a missing dependency: 'setPosition'. Either include it or remove the dependency array react-hooks/exhaustive-deps

What is the difference with use:

  const [position, setPosition] = useState({a: 1});
  useEffect(() => {
    setPosition({ left: 1, top: 1 });
  }, [position]);

and why does eslint detect a possible dependency warning but when I use a native hook there is no warning?

This is resolved now. https://github.com/facebook/react/issues/16313#issuecomment-587149109

@ger86 native hooks are already into internal git hooks. You also can move your lint into a git hook if you think you need it.

We ran into this issue… my team is new to hooks and we’re making all kind of rookie mistakes 😉 so when I turned on the new exhaustive deps rule and ran it through auto fix I had this unpleasant effect of code being utterly broken all over the place. So we have to disable the rule completely because people inadvertently run autofix and break code all over.

We’re going through and fixing all of these instances but yeah having “working” (yes I realize its broken in a sense) code all of a sudden cause fatal failures seems unacceptable for an “autofix”. I don’t think we’ll ever enable this rule with autofix because we can’t trust that it won’t produce a fatally broken build.

@EugeneDraitsev I thought eslint-disabe shoud also disable eslint --fix Googled that you can disable autofixig this rule as temporary solution: eslint --fix --rule 'react-hooks/exhaustive-deps: off'