Skip to content

Update eslint rule exhaustive deps to use new suggestions feature#17385

Merged
gaearon merged 1 commit into
react:masterfrom
wdoug:eslint-exhaustive-deps-suggestions
Feb 17, 2020
Merged

Update eslint rule exhaustive deps to use new suggestions feature#17385
gaearon merged 1 commit into
react:masterfrom
wdoug:eslint-exhaustive-deps-suggestions

Conversation

@wdoug

@wdoug wdoug commented Nov 16, 2019

Copy link
Copy Markdown
Contributor

This pull request closes #16313

Assuming that eslint/eslint#12384 moves forward and adds the suggestions feature to eslint, this pull request updates the react-hooks/exhaustive-deps eslint rule to use the suggestions feature. This will result in the rule no-longer modifying code functionality when running eslint's autofixer while providing a new method to explicitly accept code suggestions for the rule.

For more details see: #16313

@codesandbox-ci

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b496ed7:

Sandbox Source
intelligent-curie-0mjky Configuration

@sizebot

sizebot commented Nov 16, 2019

Copy link
Copy Markdown
Details of bundled changes.

Comparing: 8e74a31...b496ed7

eslint-plugin-react-hooks

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
eslint-plugin-react-hooks.development.js +0.6% +0.4% 75.25 KB 75.67 KB 17.31 KB 17.37 KB NODE_DEV
eslint-plugin-react-hooks.production.min.js 🔺+1.1% 🔺+0.6% 20.14 KB 20.37 KB 6.9 KB 6.94 KB NODE_PROD

Size changes (stable)

Generated by 🚫 dangerJS against b496ed7

@sizebot

sizebot commented Nov 16, 2019

Copy link
Copy Markdown
Details of bundled changes.

Comparing: 8e74a31...b496ed7

eslint-plugin-react-hooks

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
eslint-plugin-react-hooks.development.js +0.6% +0.4% 75.26 KB 75.68 KB 17.31 KB 17.38 KB NODE_DEV
eslint-plugin-react-hooks.production.min.js 🔺+1.1% 🔺+0.6% 20.16 KB 20.38 KB 6.91 KB 6.95 KB NODE_PROD

Size changes (experimental)

Generated by 🚫 dangerJS against b496ed7

@gaearon

gaearon commented Nov 19, 2019

Copy link
Copy Markdown
Collaborator

Nice! Please ping this PR when it lands.

@wdoug

wdoug commented Nov 22, 2019

Copy link
Copy Markdown
Contributor Author

Suggestions have been merged. Waiting on a release now.

@wdoug

wdoug commented Nov 22, 2019

Copy link
Copy Markdown
Contributor Author

Suggestions released in 6.7.0

@gaearon

gaearon commented Nov 23, 2019

Copy link
Copy Markdown
Collaborator

What happens if you have older ESLint? Do we just lose the suggestions?

Should we only fix this forward?

@wdoug

wdoug commented Nov 23, 2019

Copy link
Copy Markdown
Contributor Author

I was assuming that we would bump the major version of this package and possibly set the peer dependency of eslint to >=6.7.0. Probably adding some notes in the Readme would also be valuable.

I don't think we'd want to try to support both suggestions and the autofixer at the same time based on something like the eslint version because then the functionality would unexpectedly change when someone updated their version of eslint. Also, I don't think we want to support the autofix option going forward as it is ultimately unsafe.

I personally think it would be better to have no fixes for folks that happen to have an older version of eslint. For example, at my company, I enabled the exhaustive-deps rule and then immediately disabled it when I realized that most everyone at my company has their editors set to automatically run eslint's autofixer and we have a ton of misuses of hooks dependencies that, without manual work to address them, would cause user-facing bugs when this rule's autofix would run. Because of our current setup, it is likely that many of these bugs would end up getting out to production and affecting actual users. In our case, it would be much better to have the rule warning with no assistance fixing the issues than have the rule attempt to fix them for us without us noticing potential breaking changes.

@gaearon

gaearon commented Nov 25, 2019

Copy link
Copy Markdown
Collaborator

Just tried this with VS Code and couldn't get suggestions to show up. Does it mean extensions need to be updated?

@wdoug

wdoug commented Nov 27, 2019

Copy link
Copy Markdown
Contributor Author

Yes. I have opened this issue for the vscode extension

@gaearon

gaearon commented Nov 27, 2019

Copy link
Copy Markdown
Collaborator

We probably shouldn’t merge until mainstream extensions catch up.

@wdoug

wdoug commented Dec 11, 2019

Copy link
Copy Markdown
Contributor Author

Is there any chance we could release a beta prerelease with the suggestions feature while we wait for extensions to catch up?

@wdoug

wdoug commented Dec 11, 2019

Copy link
Copy Markdown
Contributor Author

For reference, I'm hoping that this vscode-eslint pull request will get merged and released soon.

@gaearon

gaearon commented Dec 12, 2019

Copy link
Copy Markdown
Collaborator

I’d feel comfortable releasing after VSCode plugin update is live. Can you check if there’s anything holding it back on that end?

@gaearon

gaearon commented Feb 17, 2020

Copy link
Copy Markdown
Collaborator

Can you test that the rule works as expected with this release? microsoft/vscode-eslint#814 (comment)

@gaearon

gaearon commented Feb 17, 2020

Copy link
Copy Markdown
Collaborator

This PR is published as eslint-plugin-react-hooks@2.4.0-alpha.0. Please confirm that it works.

@gaearon gaearon merged commit 93a229b into react:master Feb 17, 2020
@gaearon

gaearon commented Feb 17, 2020

Copy link
Copy Markdown
Collaborator

Works in my testing.

@gaearon

gaearon commented Feb 17, 2020

Copy link
Copy Markdown
Collaborator

eslint-plugin-react-hooks@2.4.0 is out with this fix.

@gaearon

gaearon commented Feb 17, 2020

Copy link
Copy Markdown
Collaborator

This is great work. Thank you for doing it.

@wdoug

wdoug commented Feb 17, 2020

Copy link
Copy Markdown
Contributor Author

My pleasure. It looks good on my end as well. Glad it finally got out.

@nstepien

Copy link
Copy Markdown

I prefered the rule with autofix working. Would it be possible to add an option to make it autofixable again?
The VSCode extension update with suggestions support isn't out yet, so it's making things a little bit awkward at the moment.

@LeopoldLerch

Copy link
Copy Markdown

I prefered the rule with autofix working. Would it be possible to add an option to make it autofixable again?
The VSCode extension update with suggestions support isn't out yet, so it's making things a little bit awkward at the moment.

Thing is, that is a common rule of eslint, that it does not change the code in regards of how it works. Which it in fact did with the autofix of the dependencies, expecially with useeffect. Altough a opt-in option for readding autofix could work, there is still the question if it SHOULD be there at all.

The update of the extension is already on the way, you can install it using @next . Regular version should be updated on the weekend as there corresponding thread of the vscode extension suggests.
In the meanwhile you can still use the prior-version of the eslint-ruleset.

@gaearon

gaearon commented Feb 18, 2020

Copy link
Copy Markdown
Collaborator

@nstepien You don't have to upgrade. :-) Feel free to revert to 2.3.0. We got this out early because #16313 has been causing pain for a while, and it can't wait more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ESLint]react-hooks/exhaustive-deps rule autofix modifies code function, violating eslint best practices

6 participants