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

[EugeneDraitsev]

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

I want to report a bug

What is the current behavior?

eslint --fix trying to break my hook)

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

I have a very simple hook:

import { useEffect, useState } from 'react'

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

  useEffect(() => {
    setTab(currentTab + 1 >= tabsAmount ? 0 : currentTab + 1)
  }, [tabsAmount, trigger])

  return [currentTab, setTab]
}

If props tabsAmount or trigger will change, I need to increase currentTab value by 1. It works fine and looks ok for me, but in eslint-plugin-react-hook rule react-hooks/exhaustive-deps will warn here:

React Hook useEffect has a missing dependency: 'currentTab'. Either include it or remove the dependency array. You can also do a functional update 'setTab(c => ...)' if you only need 'c
urrentTab' in the 'setTab' call react-hooks/exhaustive-deps

And eslint --fix will break my code like this:

import { useEffect, useState } from 'react'

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

  useEffect(() => {
    setTab(currentTab + 1 >= tabsAmount ? 0 : currentTab + 1)
  }, [currentTab, tabsAmount, trigger])

  return [currentTab, setTab]
}

It will add currentTab in deps for useEffect and this will create endless loop.

What is the expected behavior?

Eslint shouldn't fix this warning with --fix option, It may break the code.

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

react: 16.8.5
eslint-plugin-react-hooks: 1.6.0

[infodusha]

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]
}

[EugeneDraitsev]

@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)

[infodusha]

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

[EugeneDraitsev]

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

[infodusha]

@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'

[gaearon]

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.

[behzad888]

@EugeneDraitsev
First, I think you shouldn't use setTab into useEffect because useEffect became for simplified some React's lifecycle methods such as componentDidMount, componentDidUpdate, and componentWillUnmount. Please read this for more information.
Second, Calling setState() in React’s lifecycle methods requires a certain level of caution. There are a few methods where calling setState() would lead to undesirable results and others where it should be avoided completely.

• componentDidMount
  componentDidMount() is invoked immediately after a component is mounted. You may call setState() immediately in componentDidMount(). It will trigger an extra rendering, but it will happen before the browser updates the screen thus render() will be called twice.

• componentDidUpdate
  componentDidUpdate() is invoked immediately after updating occurs. You may call setState() immediately here but know that it must be wrapped in a condition.

• componentWillUnmount
  You should not call setState() here because the component will never be re-rendered. Once a component instance is unmounted, it will never be mounted again.

So, I think if you want to use setTab into useEffect it must be wrapped in a condition.

[EugeneDraitsev]

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

[gunn4r]

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.

[ger86]

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?

[behzad888]

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

[kpaxqin]

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.

Here is my case : #16006

Disable autofix by default sounds good for me

[nitzansan]

@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?

[humbkr]

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.

[xiel]

@gaearon The autofix is still causing problems when adding this rule to a existing code base. Yes, there are potential problems in the code, which should be addressed, but this "fix" breaks the code (sometimes) immediately.

[EugeneDraitsev]

I renamed this issue to avoid misunderstanding of it. Maybe we finally can get a decision about this problem and close it.

[AndrewCraswell]

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.

[gaearon]

Closing as a duplicate of #16313.
The solution to this will be #17385.

[gaearon]

This is resolved now.
#16313 (comment)

[youyouliangshao]

Sounds goods, I met the same problem