search

un-ts / eslint-plugin-import-x

`eslint-plugin-import-x` is a fork of `eslint-plugin-import` that aims to provide a more performant and more lightweight version of the original plugin.
https://npm.im/eslint-plugin-import-x
MIT License
368 stars 18 forks source link

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

Open error-four-o-four opened 2 months ago

error-four-o-four commented 2 months ago

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

error-four-o-four commented 2 months ago

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

SukkaW commented 2 months ago

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

error-four-o-four commented 1 month ago

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 commented 1 month ago

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

silverwind commented 1 month ago

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 commented 1 month ago

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

silverwind commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

michaelfaith commented 1 month ago

@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 commented 1 month ago

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.