Following the ESLint 9 support, import/no-unused-modules only works in flat config if an .eslintrc file exists as well

[ronbraha]

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 the import/no-unused-modules with the existing unusedExports option results in the ESLint CLI not recognizing the eslint.config.js unless there's a .eslintrc file next to it (the .eslintrc file can be empty; it just ignores it).

Oops! Something went wrong! :(

ESLint: 9.11.1

ESLint couldn't find a configuration file. To set up a configuration file for this project, please run:

    npm init @eslint/config@latest

ESLint looked for configuration files in <project rootDir> and its ancestors. If it found none, it then looked in your home directory.

If you think you already have a configuration file or if you need more help, please stop by the ESLint Discord server: https://eslint.org/chat

As long as the .eslintrc exists, the rule works as expected.

[michaelfaith]

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?

[KristjanTammekivi]

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

[zoran995]

@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

• removing the .eslintrc from the root of the repo
• moving flat example to it's own directory outside of the project

[ljharb]

Sounds like we need a dynamic test case to cover this scenario.

[michaelfaith]

Thanks for the repo, that's helpful. It looks like it's blowing up when we're calling the og FileEnumerator's iterateFiles function.

eslint-plugin-import/src/rules/no-unused-modules.js

Lines 65 to 68 in 67cc798

	return Array.from(
	e.iterateFiles(src),
	({ filePath, ignored }) => ({ filename: filePath, ignored }),
	);

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.

[michaelfaith]

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?

[zoran995]

@michaelfaith If I got the code correctly it shouldn't even try to use FileEnumerator due to this check

eslint-plugin-import/src/rules/no-unused-modules.js

Lines 165 to 171 in 67cc798

	if (
	context.session
	&& context.session.isFileIgnored
	&& context.session.isDirectoryIgnored
	) {
	return listFilesWithModernApi(src, extensions, context.session);
	}

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

[michaelfaith]

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

[ljharb]

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.

[ronbraha]

@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

[michaelfaith]

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

https://github.com/eslint/eslint/blob/d0a5414c30421e5dbe313790502dbf13b9330fef/lib/cli-engine/file-enumerator.js#L218-L222

That factory class has a useEslintrc parameter that defaults to true.

https://github.com/eslint/eslintrc/blob/fc9837d3c4eb6c8c97bd5594fb72ea95b6e32ab8/lib/cascading-config-array-factory.js#L211

This is where it throws the config error (but only if useEslintrc is true)

https://github.com/eslint/eslintrc/blob/fc9837d3c4eb6c8c97bd5594fb72ea95b6e32ab8/lib/cascading-config-array-factory.js#L519-L522

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?

[ljharb]

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.

[michaelfaith]

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 (eslint/eslint#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?

[ljharb]

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.

[phun-ky]

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

[ljharb]

Not the plugin, but this one rule, yes, it seems that's the case.

[osbornk]

I just discovered the same case with import/no-default-esport

[guillaumebrunerie]

Maybe the need of an empty .eslintrc should be documented in the mean time?

[TheJaredWilcurt]

import/no-default-esport

I set my default esport to Melee.

[guillaumebrunerie]

Maybe the need of an empty .eslintrc should be documented in the mean time?

Actually, the .eslintrc being still used by the no-unused-modules rule to determine which files should be included, it should probably not be empty but instead contain a reasonable ignorePatterns. I just checked in my current project, and not having ignorePatterns there makes my total lint time go from 15 seconds to 45 seconds.

[KristjanTammekivi]

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 (eslint/eslint#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?

Have You had any time to explore it? (aka bump)

I would do it myself, but I'm not very familiar with either this repo or eslint internals.

[michaelfaith]

Have You had any time to explore it? (aka bump)

I would do it myself, but I'm not very familiar with either this repo or eslint internals.

Unfortunately not; been too busy. I'll try to make time for it in the next month or two. Happy to review anything that you want to take a stab at though. Anything that you'd need to know from this repo is pretty isolated to the one file for this rule, and i linked to some of the relevant places in the eslint repo. So the breadcrumbs are there, if you want to give it a shot.

[michaelfaith]

I finally had some time to work on this this morning: #3116

Note: This doesn't fix the fact that the FileEnumerator doesn't have knowledge of what the user's config is ignoring, it just prevents the rule from looking for a legacy / rc config and erroring out.

[guillaumebrunerie]

Wouldn’t it be better to error out with a better error message when using no-unused-modules and no .eslintrc? The fact that ignore patterns need to be in a .eslintrc isn’t obvious at all (especially if it doesn’t error out anymore) and can have a pretty dramatic impact on performance.

[guillaumebrunerie]

it just prevents the rule from looking for a legacy / rc config

Does that mean that you are actually removing support for ignore patterns in the no-unused-modules rule when using flat config?

If so, I don’t think that's a good idea at all. The current situation is not ideal (even if you use flat config, you still need to keep a .eslintrc with ignore patterns) but at least it works. Removing support for ignore patterns entirely will make this plugin unusable again with flat config in projects that need to ignore build files. For instance, as I mentioned above, this change would make my lint time go from 15 seconds to 45 seconds…

[michaelfaith]

it just prevents the rule from looking for a legacy / rc config

Does that mean that you are actually removing support for ignore patterns in the no-unused-modules rule when using flat config?

This is true. I guess I could change it to try again in a catch block if it errors out the first time, so that if you do have an eslintrc it doesn't change the behavior. There's not really a way to get the old FileEnumerator api to be aware of ignores in flat config, without completely re-implementing the ignore logic in eslint. So it's either an eslintrc is required for this rule to know about ignore patterns, or it doesn't try to look for an eslintrc at all (which is what my PR moves it to). Neither is great, but the later seemed more "correct". @ljharb thoughts?

[michaelfaith]

I guess I could change it to try again in a catch block if it errors out the first time, so that if you do have an eslintrc it doesn't change the behavior.

@guillaumebrunerie or are you suggesting we don't even go this far, and instead just accept that eslintrc is required for this rule and log a more expressive error? If so, i'm ok with that, but then why'd we even wait for me to explore #3079 (comment) I made it clear that whatever came of that wouldn't support ignored files.

[guillaumebrunerie]

@guillaumebrunerie or are you suggesting we don't even go this far, and instead just accept that eslintrc is required for this rule and log a more expressive error?

Yes, that's what I would suggest.
It seems to me that if someone wants to use no-unused-modules, they probably want to know how to make it ignore files, so it's better to fail if no .eslintrc is present with a clear error message like:

Due to some restrictions in ESLint 9, the no-unused-modules rule requires a
.eslintrc file to know which files to ignore (even when using flat config).
The .eslintrc file only needs to contains "ignorePatterns", or can be empty
if you do not want to ignore any files.
Check https://github.com/import-js/eslint-plugin-import/issues/3079 for more
context.

[ljharb]

Hmm. I think that means that #3116 is a breaking change.

Following the above suggestion would indeed prevent a crash and still have it work when an eslintrc is present, but that’s weird and we shouldn’t be aiming for that as a goal state. However, it would be a better intermediate state than the status quo.

[michaelfaith]

Following the above suggestion would indeed prevent a crash and still have it work when an eslintrc is present

It wouldn't really prevent a crash though, if the rc file isn't present, it would just emit a better / more informative error message, right?

[ljharb]

ah, fair point. 🤔

[michaelfaith]

I updated #3116 with what we talked about. It no longer attempts to avoid the error and instead throws an error with a more meaningful message if flat config is enabled and FileEnumerator errored out due to a lack of rc file.