Open ronbraha opened 1 week ago
This is not something that's come up before. There are issues with no-unused-modules
in flat config, but worst case scenario is that it should silently fail (or not ignore things that are specified to ignore). It shouldn't force you to use an rc config. We have this rule set up in the flat config example of this repo, if you want to compare your set up with that one. If that doesn't shed any light on your issue, would you mind sharing your config and the command you're using to run eslint? We'll need to try and recreate the issue. And just to confirm you don't have ESLINT_USE_FLAT_CONFIG
environment variable set to false anywhere?
Hi, can confirm that this is the case. I had a test-repo ready to go, here it is:
https://github.com/KristjanTammekivi/eslint-plugin-import-x-flat-config-debug
npm run lint # error ESLint couldn't find a configuration file.
touch .eslintrc
npm run lint # linting works, shows unused exports
@michaelfaith In the examples eslint find .eslintrc in the root of the repo and it satisfies the requirements and linting passes. You can check that it fails by
Sounds like we need a dynamic test case to cover this scenario.
Thanks for the repo, that's helpful. It looks like it's blowing up when we're calling the og FileEnumerator
's iterateFiles
function.
So, my first thought is that maybe eslint put more strict controls in place to prevent using that deprecated api in flat config. However, what's really vexing, is I'm struggling to re-create it in this repo. We have a flat config example, and even after updating to the latest eslint (the same version as your repo uses), both at the root of the project and in the example project, those same lines of code work fine. And I double checked that the instance of FileEnumerator
in both cases is coming from 9.11 eslint in node_modules
(so it's not some weird multiple installed versions issue).
I'll keep trying to reproduce it in this repo to either confirm or debunk that initial suspicion.
Well damn. It just occurred to me, while this repo doesn't have an rc config in the example folder I'm running the tests in, it does have one at the root of the repo that it uses for its own linting. And as we know the way rc config climbs the directory tree to merge configs means it was satisfied by the presence of the root rc config being there. So this issue was flying under the radar. At least we know it's not some funky issue with this repo's version set up.
@ljharb how would you like to approach this? It doesn't seem as though we can fallback on the deprecated FileEnumerator
API in v9 after all, and it sounds like there's some issue with moving forward with that original API proposal that we were counting on moving forward. It seems to be satisfied if the rc file simply exists, but that's a pretty clunky experience. We could just have it no-op if the flat config is detected, so it doesn't even try to use the old FileEnumerator
?
@michaelfaith If I got the code correctly it shouldn't even try to use FileEnumerator due to this check https://github.com/import-js/eslint-plugin-import/blob/67cc79841fc823ad4af2532af2dc6704e4b3b03a/src/rules/no-unused-modules.js#L165-L171 but the session is not available on the context object and it seems it was just available as part of POC created back in February but never got merged into eslint codebase (https://github.com/eslint/eslint/issues/18087#issuecomment-2321443857)
Right. It was a bit of a chicken and egg situation. They didn't want to move forward with adding those new functions to the session until we demonstrated demand. So we added that solution in so that the proposal could move forward (knowing that we'd possibly have to tweak it, if the final api landed in a slightly different form), but still fall back on the deprecated api until it was available. But it seems they've pulled back on that plan (i think).
It'd be better for the rule to error in flat config than silently noop, but even better still to figure out how to get it working.
@KristjanTammekivi, thanks for the example repo! I saw many discussions around the no-unused-modules
rule, so I am glad I raised something you were unaware of.
Just for more context: I also experienced this issue in ESLint 8 with a flat config, but I assumed it was a known issue, so I waited for an ESLint 9 support to re-test it
It'd be better for the rule to error in flat config than silently noop, but even better still to figure out how to get it working.
I think there are a couple options that we could explore, but I don't think any of them are great. We could possibly patch FileEnumerator
. It takes a config file factory as a constructor param, which is responsible for finding the config (to use for understanding what to ignore).
That factory class has a useEslintrc
parameter that defaults to true.
This is where it throws the config error (but only if useEslintrc
is true)
So that could provide a path to get around it, but honestly i question the value of going that route, since it continues to rely on FileEnumerator
, which we know will go away in the next major version. So, spending that kind of energy on a temporary band-aid doesn't seem great, plus the FileEnumerator
solution, when it doesn't hit this issue and the rule runs, it still has the issues with ignored files we found. Since that config factory can't find an rc config, it doesn't know what to ignore.
Until there's a first-class api added to eslint, like the original plan involved, I don't really see a great option other than throwing a (better) error. What do you think?
I agree a temporary band-aid isn't great, but it would buy us lots of time because eslint 10 would need to come out and it always takes the ecosystem many months to catch up to eslint - so it's not a terrible idea.
If we have a need for a first-class API in eslint, that we can't polyfill/implement ourselves, then that's evidence we'd want to provide to eslint maintainers.
I agree a temporary band-aid isn't great, but it would buy us lots of time because eslint 10 would need to come out and it always takes the ecosystem many months to catch up to eslint - so it's not a terrible idea.
Sounds good. I can explore that, if nobody else beats me to it.
If we have a need for a first-class API in eslint, that we can't polyfill/implement ourselves, then that's evidence we'd want to provide to eslint maintainers.
I think your original issue / feature request (https://github.com/eslint/eslint/issues/18087) covered everything. @nzakas seems unsupportive of covering this use case after the original proposal didn't move forward, and I'm not sure that we have any new information to share?
I think it's best to wait this time until we've built our own replacement, and then we can clearly articulate what we need from eslint, and what the complexity addition to core buys us in terms of correctness, performance, maintainability, other plugins using it, etc.
So, just to be clear, this plugin is unusable until we have a fix for this? Or does it currently only require a dummy .eslintrc
file in the project root, but it will read the eslint.config.mjs
file?
EDIT: Can confirm, it requires an empty .eslintrc
..
Not the plugin, but this one rule, yes, it seems that's the case.
I have seen various discussions about this rule, but not quite in the exact same context, so I am unsure if that could be merged with an existing issue:
I upgraded to
eslint-plugin-import
v2.31.0, which officially supports ESLint V9. Enabling theimport/no-unused-modules
with the existingunusedExports
option results in the ESLint CLI not recognizing theeslint.config.js
unless there's a.eslintrc
file next to it (the.eslintrc
file can be empty; it just ignores it).As long as the
.eslintrc
exists, the rule works as expected.