eslint-plugin-import-x icon indicating copy to clipboard operation
eslint-plugin-import-x copied to clipboard

[Bug?] no-unused-modules (and deprecation)

Open error-four-o-four opened this issue 1 year ago • 12 comments

Hey there,

so there are two possibilities: either I'm completely confused and can't see what I'm doing wrong or this is a real bug.

Setting the rule 'import-x/no-unused-modules' results in the following error in the output of vscode:

No ESLint configuration (e.g .eslintrc) found for file: .\eslint-config-404\eslint.config.js
File will not be validated. Consider running 'eslint --init' in the workspace folder eslint-config-404
Alternatively you can disable ESLint by executing the 'Disable ESLint' command.

And the usual one in the cli:

Oops! Something went wrong! :(
ESLint: 9.4.0
ESLint couldn't find a configuration file. To set up a configuration file for this project, please run: ...

While using the following setup:

node -v v20.9.0
VS Code ESLint extension v2.4.4
"@eslint/eslintrc": "^3.1.0",
"eslint": "^9.4.0",
"eslint-plugin-import-x": "^0.5.1"

Here's a reproduction of the error: https://github.com/error-four-o-four/eslint-config-404/blob/repro-no-unused-modules/eslint.config.js

Additionally

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • I have read the FAQ

error-four-o-four avatar Jun 11 '24 05:06 error-four-o-four

Apologies. I'm still new to the whole eslint ecosystem and I'm slowly digging deeper into it.

The issue is related to https://github.com/eslint/eslint/issues/18087

error-four-o-four avatar Jun 12 '24 15:06 error-four-o-four

@silverwind

ESLint might add multi-file analysis support in the future. But for now, ESLint works on one rule at a time, where there is no concept of information from one file being available in another file. Thus I am considering deprecating the no-unused-modules rule in favor of tools like knip. I am not planning to remove this rule completely as ESLint might have first-party cross-file reference support in the remote future.

Since you are contributing https://github.com/import-js/eslint-plugin-import/pull/3011 (and https://github.com/un-ts/eslint-plugin-import-x/issues/86), I'd like to hear about your ideas about this rule.

Contexts:

  • https://github.com/eslint/eslint/issues/18087
  • https://github.com/import-js/eslint-plugin-import/pull/3018#issuecomment-2195340793
  • https://github.com/import-js/eslint-plugin-import/pull/3018#issuecomment-2203997012

SukkaW avatar Jul 08 '24 07:07 SukkaW

Hey there,

I'm afraid I can't contribute something useful. I'm still in the process of getting familiar with the whole eslint ecosystem and I haven't heard of knip before but might try it in the near future. Deprecating 'no-unused-modules' would be fine.

Cheers

error-four-o-four avatar Jul 11 '24 22:07 error-four-o-four

I can't judge knip until it actually works for me (https://github.com/webpro-nl/knip/issues/725).

silverwind avatar Jul 11 '24 23:07 silverwind

And no, I don't think it's a good idea to deprecate a lint rule because that tool exists. The rule is focused on just finding unused code while that tool does check many more things and seems to have problems with false-positives, so will require fiddling with config.

Also, it adds 15 dependencies totalling almost 6MB which is too much for me personally.

silverwind avatar Jul 12 '24 07:07 silverwind

In https://github.com/eslint/eslint/issues/18087 they proposed a solution but ljharb didn't have a repo to test it out so the eslint changes weren't shipped in time (https://github.com/eslint/eslint/issues/18087#issuecomment-1964809741). Could a maintainer here perhaps validate the proposal and reply there?

As for knip - sadly it doesn't have vscode support like eslint does (ie highlighting what's unused inside a file), also not looking forward to adding and configuring a new dependency in umpteen microservices, so I'm happy that You're not considering removing the rule completely.

edit: knip does work fast though, I'll give them that

KristjanTammekivi avatar Jul 18 '24 07:07 KristjanTammekivi

For my personal config, I will just disable no-unused-modules because I find the value it provides to be rather minimal and I end up fighting it with eslint-disable comments more often then cases where it has provided actual benefit in finding unused code.

silverwind avatar Jul 18 '24 11:07 silverwind

Could a maintainer here perhaps validate the proposal and reply there?

Hmmm, actually eslint-plugin-import-x already has no-unused-modules working with the flat config under ESLint 8.x by importing FileEnumerator from eslint/use-at-your-own-risk (ljharb can't do that because he wants compatibility down to ESLint 3, LMAO). We already added a unit test to ensure that works.

In the long term, we want ESLint to provide official cross-file reference support.

SukkaW avatar Jul 18 '24 11:07 SukkaW

Could a maintainer here perhaps validate the proposal and reply there?

Hmmm, actually eslint-plugin-import-x already has no-unused-modules working with the flat config under ESLint 8.x by importing FileEnumerator from eslint/use-at-your-own-risk (ljharb can't do that because he wants compatibility down to ESLint 3, LMAO). We already added a unit test to ensure that works.

In the long term, we want ESLint to provide official cross-file reference support.

I was using 0.5.something, saw that new version was out and hoped that that would fix it but no, I still get the error.

I made a barebones example of what I'm doing here https://github.com/KristjanTammekivi/eslint-plugin-import-x-flat-config-debug/tree/main I would be very grateful if You could have a look at it

KristjanTammekivi avatar Jul 18 '24 12:07 KristjanTammekivi

For my personal config, I will just disable no-unused-modules because I find the value it provides to be rather minimal and I end up fighting it with eslint-disable comments more often then cases where it has provided actual benefit in finding unused code.

Actually I thought again and will give the rule another chance once https://github.com/un-ts/eslint-plugin-import-x/pull/116 is in. It is a useful rule when doing big refactors and I' definitely want to keep these checks in eslint, not some other tool (reasons: I don't want any more dependencies and I want editor integration).

silverwind avatar Jul 18 '24 21:07 silverwind

@silverwind

ESLint might add multi-file analysis support in the future. But for now, ESLint works on one rule at a time, where there is no concept of information from one file being available in another file. Thus I am considering deprecating the no-unused-modules rule in favor of tools like knip. I am not planning to remove this rule completely as ESLint might have first-party cross-file reference support in the remote future.

Since you are contributing import-js#3011 (and #86), I'd like to hear about your ideas about this rule.

Contexts:

I concur with the assertions mentioned in the referenced comments. It definitely feels like a case of trying to force a tool to do something it wasn't intended to do. I'm a big proponent of using the right tool for the right job, and knip works great for what it does. Just my two cents. With that said, i don't mind porting over that implementation, since it's pretty straightforward to do. But just wanted to weigh in and raise the "should" vs "could" question to consider.

michaelfaith avatar Jul 26 '24 09:07 michaelfaith

Yes, I need to investigate knip further, now that https://github.com/webpro-nl/knip/issues/725 is fixed it works on a basic level, but still has a lot of false-positive issues for the other stuff it checks like dependencies. If knip can be made to check only for unused exports, it might be immedately useful for me as a replacement for this rule.

silverwind avatar Jul 26 '24 10:07 silverwind