Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: [eslint-plugin-react-hooks] - exhaustive-deps autofix not working after 2.4.0 #18235

Closed
melloc01 opened this issue Mar 6, 2020 · 20 comments
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@melloc01
Copy link

melloc01 commented Mar 6, 2020

Hi there, after upgrading to the latest version the autofix does not automatically includes the missing deps, is this the expected behavior? I didn't find any release notes of the lib so I couldn't check if that was expected, also, I'd say, if done on purpose, this change should be on a major version, shouldn't it?

The current behavior 2.4.0 or above

Some deps are missing.
image

Quick fix shows:
image
eslint --fix shows:
image

But code is not being updated, although if I manually click it add the missing deps.

Expected behavior, 2.3.0

Noticed it shows another context menu:
image

Also it does add the missing deps using the same eslint --fix command.

@melloc01 melloc01 added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Mar 6, 2020
@sopretty
Copy link

sopretty commented Mar 6, 2020

@melloc01 I'm facing exactly the same problem, I had to use eslint CLI to add them automatically.

melloc01 added a commit to intelie/eslint-config that referenced this issue Mar 6, 2020
@itsmylife
Copy link

I have the same problem. IntelliJ Idea is not showing me autofix options anymore.
But VS Code is showing the autofix.

package.json

 "dependencies": {
    "@zeit/next-sass": "^1.0.1",
    "next": "9.3.0",
    "node-sass": "^4.13.1",
    "react": "16.13.0",
    "react-dates": "^21.8.0",
    "react-dom": "16.13.0",
    "react-redux": "^7.2.0",
    "redux": "^4.0.5"
  },
  "devDependencies": {
    "@types/node": "^13.9.0",
    "@types/react": "^16.9.23",
    "@types/react-dates": "^17.1.10",
    "@types/react-dom": "^16.9.5",
    "@types/react-redux": "^7.1.7",
    "@typescript-eslint/eslint-plugin": "^2.23.0",
    "@typescript-eslint/parser": "^2.23.0",
    "eslint": "^6.8.0",
    "eslint-plugin-react": "^7.19.0",
    "eslint-plugin-react-hooks": "^2.5.0",
    "eslint-plugin-typescript": "^0.14.0",
    "typescript": "^3.8.3",
    "typescript-eslint-parser": "^22.0.0",
    "webpack-filter-warnings-plugin": "^1.2.1"
  }

.eslintrc

{
  "parser": "@typescript-eslint/parser",
  "plugins": [
    "@typescript-eslint",
    "react-hooks"
  ],
  "extends": [
    "eslint:recommended",
    "plugin:@typescript-eslint/eslint-recommended",
    "plugin:@typescript-eslint/recommended",
    "plugin:react/recommended"
  ],
  "parserOptions": {
    "ecmaVersion": 2020,
    "sourceType": "module",
    "jsx": true
  },
  "settings": {
    "react": {
      "version": "detect"
    }
  },
  "rules": {
    "react-hooks/rules-of-hooks": "error",
    "react-hooks/exhaustive-deps": "error"
  }
}

VS Code:
image

IntelliJ Idea:
image

@gaearon
Copy link
Collaborator

gaearon commented Mar 12, 2020

This is not a bug. We disabled the autofix because it created issues and wasn't consistent with people's expectations for ESLint autofix. Instead, we implemented the same functionality using the ESLint Suggestions API which was added in ESLint 6.7.0. You'll need to wait for your IDE to support it: #18099 (comment)

VS Code ESLint plugin already supports it.

@gaearon gaearon closed this as completed Mar 12, 2020
@guico33
Copy link

guico33 commented Mar 18, 2020

Not sure I am following here, I am using vscode and can now semi-manually fix the issue through the UI (understand that eslint will fill the dep array automatically if I click to fix it as shown in #18235 (comment)).

Is there a way for this to be done automatically as part of auto-fixable problems like it used to be?

I have auto-fix on save enabled and I find it saves me a lot of time compared to clicking the UI to fix the problem.

As a side note, I barely ever had problems with the suggested fix before, one may always disable the rule for the line if needed.

@gaearon
Copy link
Collaborator

gaearon commented Mar 18, 2020

Is there a way for this to be done automatically as part of auto-fixable problems like it used to be?

No, and intentionally so. This fix changes the behavior — in some cases, potentially introducing infinite loops. It's not safe to apply automatically. You need to read the suggestion and check if it makes sense. (When it doesn't, you usually need to fix other code rather than disable the rule btw.)

@gaearon
Copy link
Collaborator

gaearon commented Mar 18, 2020

it saves me a lot of time compared to clicking the UI to fix the problem.

I think Cmd+. expands suggestions in VS Code so you don't have to click. Regardless, this is a UI problem that VS Code can solve, e.g. by adding an easier way to iterate through a list of problems.

@melloc01
Copy link
Author

Yes.. I stumbled upon the "infinite loop" problem caused by this rule quite a few times.

While it had its issues, we programmers tend to get used to it and use it in our favor - (Ok! Fill it for me and I'll review it) - but I agree it should be a suggestion, it is just that we have to work on that muscle memory and get used to the new behavior.

Thanks @gaearon

@gaearon
Copy link
Collaborator

gaearon commented Mar 31, 2020

I'm adding autofix back behind an appropriately named option for people who really need it. Note: if your IDE or lint setup actually fixes it automatically, you do not want this. This is only for people who use older IDEs that don't yet support Suggestions and who don't run this as an automatic step.

#18437

@FrimJo
Copy link

FrimJo commented Apr 16, 2020

Hi @gaearon!

In your PR #18437 you write:

If you actually apply fix suggestions automatically, you don't want to enable this.

and in your comment above you write that you are adding autofix back, which confuses me a bit.

I'm currently autofixinig my lint errors using

"editor.codeActionsOnSave": {
  "source.fixAll": true,
},

which works wonders for all my lint errors except react-hooks/exhaustive-deps.

I have "eslint-plugin-react-hooks": "^3.0.0" and latest version of vscode-eslint.

Now, if I want to have autofix for exhausive deps, should I add enableDangerousAutofixThisMayCauseInfiniteLoops? And how do I add it?

I've tried

"rules": {
  ...
  "react-hooks/exhaustive-deps": [1, { "enableDangerousAutofixThisMayCauseInfiniteLoops ": true }],
  ...
}

But ESLint just tells me

Configuration for rule "react-hooks/exhaustive-deps" is invalid: Value {"enableDangerousAutofixThisMayCauseInfiniteLoops":true} should NOT have additional properties.`

I've also tried

"rules": {
  ...
  "react-hooks/exhaustive-deps": [{ "enableDangerousAutofixThisMayCauseInfiniteLoops ": true }],
  ...
}

But get

Configuration for rule "react-hooks/exhaustive-deps" is invalid: 	Severity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '{ "enableDangerousAutofixThisMayCauseInfiniteLoops ": true }').

@FrimJo
Copy link

FrimJo commented Apr 16, 2020

Maybe we need this PR #18555 by @osdnk to go through first? It's currently blocked.

@osdnk
Copy link

osdnk commented Apr 23, 2020

ping @gaearon

@gaearon
Copy link
Collaborator

gaearon commented Apr 23, 2020

@FrimJo

If you actually apply fix suggestions automatically, you don't want to enable this.

and in your comment above you write that you are adding autofix back, which confuses me a bit.

I am only adding it back behind a scary option because at FB, we have a custom ESLint VS Code extension that:

  1. never applies "autofix" on save
  2. only supports "autofix" as interactive suggestions (and doesn't support Suggestions API yet)

So I'm adding this as a workaround, but we only use it because we don't autofix on save.

@gaearon
Copy link
Collaborator

gaearon commented Apr 23, 2020

I'm currently autofixinig my lint errors using

Again, you don't want to "autofix" this rule on save. This is a mistake. It will lead to problems. This rule is meant to be applied manually, with careful consideration in each case.

(The concrete reason this option doesn't work though is because we haven't cut a release with it yet)

@AlicanC
Copy link
Contributor

AlicanC commented Jul 10, 2020

This isn't a mistake worse than typing while (true) {} and hitting save.

When we are working with hooks with deps, we write an empty deps array, hit save, check how our code works, and if it causes an infinite loop or something we disable the rule and manually manage the deps.

We have an app with 358 components (all using hooks) and we disabled the rule just 3 times. The autofix works almost all the time and when it doesn't work it never causes anything catastrophic.

If people are autofixing on commit/push/etc. and debugging for hours, I'd say the problem lies with their DX. The errors should always appear on the IDE and autofixing should always happen on save and not magically with a commit hook or something.

I understand that this isn't really an autofix since autofixes should not change functionality, but there's no other way to do this as easy as turning on enableDangerousAutofixThisMayCauseInfiniteLoops and hitting save.

If we didn't have enableDangerousAutofixThisMayCauseInfiniteLoops our DX with hooks would be significantly worse, so many many thanks for spending the time bringing this back and please keep supporting it until VSCode provides a way to apply the suggestion on save.

@Dema
Copy link

Dema commented Oct 18, 2020

@FrimJo Just in case if you didn't find why it's not working or for the benefit of others who will try it — there is a SPACE after option name "enableDangerousAutofixThisMayCauseInfiniteLoops "! If you remove it or remove quotes, everything will work.

@azz0r
Copy link

azz0r commented Jan 28, 2021

Any update on this?

@Dema
Copy link

Dema commented Jan 28, 2021

@azz0r What exactly are you expecting to be updated? They started using code actions for this fix, if you want an old behavior, enable enableDangerousAutofixThisMayCauseInfiniteLoops option in eslint config.

@KamalAman
Copy link

KamalAman commented Aug 13, 2021

From Amcsi comment in CRA:

They made it so react-hooks/exhaustive-deps does not autofix anymore by default.

To get autofixing to function add the following rule to your eslint

{
  ...
  "rules": {
    "react-hooks/exhaustive-deps": [
       "warn",
        {
          "enableDangerousAutofixThisMayCauseInfiniteLoops": true
        }
      ]
    }
  },

It works as well as it ever did!

@SrBrahma
Copy link

I only had infinite loop once through those years and the autofix was off (happened some days ago), I've just accepted the suggestion automatically without questioning as my muscular memory taught me after the autofix was remove lol

Activated the autofix because the error is easy to spot and to know where it's happening, and it's very rare to happen. For my case, it was a key generator for each render; so I changed from state + 1 to Date.now().

@parkerault
Copy link

parkerault commented May 5, 2022

Sorry to necropost, but one thing that I haven't seen discussed in this thread: the autofix will overwrite any dependencies already in place, so if you have one that isn't used in the effect, but you're using to trigger the effect, you'll lose it. This is kind of a weird example, but:

React.useEffect(() => {
  console.log(`networkStatus for ${entity} changed.`);
}, [networkStatus]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests