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?] no-unused-modules (and deprecation) #90

Open
error-four-o-four opened this issue Jun 11, 2024 · 12 comments
Open

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

error-four-o-four opened this issue Jun 11, 2024 · 12 comments

Comments

@error-four-o-four
Copy link

error-four-o-four commented Jun 11, 2024

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
Copy link
Author

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

The issue is related to eslint/eslint#18087

@SukkaW
Copy link
Collaborator

SukkaW commented Jul 8, 2024

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

@SukkaW SukkaW changed the title [Bug?] no-unused-modules [Bug?] no-unused-modules (and deprecation) Jul 10, 2024
@error-four-o-four
Copy link
Author

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

@silverwind
Copy link

I can't judge knip until it actually works for me (webpro-nl/knip#725).

@silverwind
Copy link

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.

@KristjanTammekivi
Copy link

KristjanTammekivi commented Jul 18, 2024

In eslint/eslint#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 (eslint/eslint#18087 (comment)). 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

@silverwind
Copy link

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.

@SukkaW
Copy link
Collaborator

SukkaW commented Jul 18, 2024

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.

@KristjanTammekivi
Copy link

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

@silverwind
Copy link

silverwind commented Jul 18, 2024

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 #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).

@michaelfaith
Copy link

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

@silverwind
Copy link

Yes, I need to investigate knip further, now that webpro-nl/knip#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants