Change Request: Flat Config does not support FileEnumerator

[ljharb]

ESLint version

v8

What problem do you want to solve?

eslint-plugin-import uses FileEnumerator in its no-unused-modules rule, for the purpose of gathering a list of files are not eslintignored, or ignored by the user's rule config, for checking if any exports or modules are unused.

It seems that in flat config, this capability does not exist.

What do you think is the correct solution?

Something that may work nicely is a new method on context provided to rules, that can achieve the same goal, but I have no idea if this makes sense for eslint or not.

Participation

• I am willing to submit a pull request for this change.

Additional comments

This is the sole remaining blocker (afaik) to eslint-plugin-import supporting Flat Config, after which I plan to do a breaking change to drop older eslint versions, which is something quite a lot of users have been asking for.

[nzakas]

eslint-plugin-import uses FileEnumerator in its no-unused-modules rule, for the purpose of gathering a list of files are not eslintignored, or ignored by the user's rule config, for checking if any exports or modules are unused.

If I'm reading this correctly, your use case is basically to answer the question "is this file ignored by ESLint?" So perhaps a method like isFileIgnored() where you can pass in the file path would address your needs?

[ljharb]

Sort of - that would require us to enumerate every file on disk and ask eslint about each one, because we also don't know where the eslint "root" is. We specifically need a list of lintable files.

[nzakas]

Okay, then I feel like there's not enough information in the original description to really understand how you're using FileEnumerator. So, can you add more detail?

And when you say "root", what is it you're referring to? (Note that context.cwd is available already.)

[ljharb]

By root, i mean root: true in eslintrc (i'm not sure if that concept still exists in flat config - maybe the cwd is always the root now?)

What we need is a list or iterator of lintable files (so we can crawl them all, parse their imports and exports, and figure out which are unused).

[nzakas]

Okay, I'm going to try asking this again: can you please explain exactly the approach/algorithm that you're using right now to do this? We need to know:

1. How are you currently determining root?
2. Is that where you're searching from or somewhere else?
3. Are you only parsing the files you find referenced in an import statement, or all of the lintable files?
4. Are files parsed all up front or on-demand (or something else)?

There isn't going to be an exact replacement for FileEnumerator, so we need to understand the problem you're trying to solve, plus the way you already solved it, in order to figure out the approach going forward.

[ljharb]

oh sure, sorry if i misunderstood :-)

The code that uses FileEnumerator is here: https://github.com/import-js/eslint-plugin-import/blob/7a21f7e10f18c04473faadca94928af6b8e28009/src/rules/no-unused-modules.js#L19-L64 and https://github.com/import-js/eslint-plugin-import/blob/7a21f7e10f18c04473faadca94928af6b8e28009/src/rules/no-unused-modules.js#L173-L187

The rule's config takes a list of globs, and a list of exclusion patterns, which we apply to narrow down the files provided by the Enumerator.

The rule requires the globs all be relative to the cwd, so any root determination beyond that is up to eslint via the Enumerator.

We're parsing all lintable files; the user defines which extensions in their plugin settings in eslint config.

Files are all parsed "on demand" in the sense that it's when the rule is run, but "up front" in that it's the first thing the rule does (if we don't cache it for all future in-process rule runs after that, then oops, we should).

[nzakas]

That's helpful, thanks.

So am I understanding you correctly in that if I do something like npx eslint foo.js, no-unused-modules will still glob and parse all of the files specified in the config? Can you provide an example config and walk me through that?

Looking through the code, it seems like perhaps you could replace FileEnumerator with a generic glob utility, and then something like a context.isFileIgnored() method would allow the same behavior you have now. It looks like you're already defaulting to searching process.cwd() if src isn't specified, which you can get from context.cwd.

It doesn't appear that you need the location of the ESLint config file for any reason (what we called root in eslintrc), or am I missing something?

[ljharb]

yes, that's right - unfortunately i don't have an example config available (they're in previous employers' codebases).

using a generic glob utility would be "fine" but there'd be no guarantee it matches the glob semantics eslint uses, now and in the future.

(when i said "root" i meant the root boolean property in eslintrc; i certainly would prefer not to ever interact with config files directly)

[sam3k]

Per 2024-02-08 tsc-meeting, @nzakas @mdjermanovic will follow up on this issue.

[nzakas]

using a generic glob utility would be "fine" but there'd be no guarantee it matches the glob semantics eslint uses, now and in the future.

I think you can get pretty close. Internally, we have isDirectoryIgnored() and isFileIgnored() that we use to filter glob results, which you can see here:

eslint/lib/eslint/eslint-helpers.js

Lines 285 to 333 in 15c143f

	deepFilter(entry) {
	const relativePath = normalizeToPosix(path.relative(basePath, entry.path));
	const matchesPattern = matchers.some(matcher => matcher.match(relativePath, true));
	return matchesPattern && !configs.isDirectoryIgnored(entry.path);
	},
	entryFilter(entry) {
	const relativePath = normalizeToPosix(path.relative(basePath, entry.path));
	// entries may be directories or files so filter out directories
	if (entry.dirent.isDirectory()) {
	return false;
	}
	/*
	* Optimization: We need to track when patterns are left unmatched
	* and so we use `unmatchedPatterns` to do that. There is a bit of
	* complexity here because the same file can be matched by more than
	* one pattern. So, when we start, we actually need to test every
	* pattern against every file. Once we know there are no remaining
	* unmatched patterns, then we can switch to just looking for the
	* first matching pattern for improved speed.
	*/
	const matchesPattern = unmatchedPatterns.size > 0
	? matchers.reduce((previousValue, matcher) => {
	const pathMatches = matcher.match(relativePath);
	/*
	* We updated the unmatched patterns set only if the path
	* matches and the file isn't ignored. If the file is
	* ignored, that means there wasn't a match for the
	* pattern so it should not be removed.
	*
	* Performance note: isFileIgnored() aggressively caches
	* results so there is no performance penalty for calling
	* it twice with the same argument.
	*/
	if (pathMatches && !configs.isFileIgnored(entry.path)) {
	unmatchedPatterns.delete(matcher.pattern);
	}
	return pathMatches || previousValue;
	}, false)
	: matchers.some(matcher => matcher.match(relativePath));
	return matchesPattern && !configs.isFileIgnored(entry.path);
	}
	})).map(entry => entry.path);

The rest of the logic around filtering there is solely for the benefit of being able to output useful errors on the CLI, which is clearly something you wouldn't need to do.

So, I think if we exposed isDirectoryIgnored() and isFileIgnored() to rules, you should be able to mimic what we're doing here.

[ljharb]

I agree, that would work, thank you. The only remaining downside is that I'd basically have to copy-paste that logic, and if eslint ever changed it, it'd be out of sync. Is there no way to get direct access to eslint's glob results in the first place, even if unfiltered?

[nzakas]

The only remaining downside is that I'd basically have to copy-paste that logic, and if eslint ever changed it, it'd be out of sync.

That is a risk, for sure, but given that we use the same isDirectoryIgnored() and isFileIgnored(), those are the most likely things that would change and you'd get those passed through to you.

Is there no way to get direct access to eslint's glob results in the first place, even if unfiltered?

Unfiltered would be disastrous because you'd end up getting everything in every node_modules (that's part of the filtering).

The glob results of the current run probably won't work for you because that won't give you all of the files in the project. For example, if someone runs npx eslint foo.js, then the result of the search will only be /path/to/foo.js. We only search based on the pattern that was passed in, and if I'm understanding you correctly, you'd need more than that if you want to check if a module is unused anywhere in the project.

As I said, this isn't really functionality we want to encourage in rules, so we're trying to make the surface area to get this working for you as small as possible.

(At some point, I'd like to look at a higher level at what it might look like for ESLint to understand the dependency graph of a project.)

[ljharb]

True, basically what we'd want is "the list of paths to be linted as if someone ran eslint . in the cwd" (which is always the way i configure every project; i personally consider passing a path to be an antipattern).

[ljharb]

So, it sounds like, unless eslint can somehow expose a function that takes a path (like .) and a dir (like process.cwd()) and returns an iterator of paths, then i'd have to somehow figure out what --ext options had been passed to eslint, and then use those to grab every file from the CWD, and filter things myself with isDirectoryIgnored and isFileIgnored?

[nzakas]

--ext doesn't exist in flat config -- all that data is in the config file and therefore is respected in isDirectoryIgnored() and isFileIgnored().

Let me put together a quick POC that you can try out.

[ljharb]

Ah, thanks, that simplifies it a bit. Look forward to seeing what you come up with :-)

[nzakas]

Here's the branch:
https://github.com/eslint/eslint/tree/issue18087-poc

The way I have this set up is as a context.session object that has isDirectoryIgnored() and isFileIgnored() as methods. (This is only available when running in flat config mode.) Each method expects an absolute path to be passed in.

Let me know how that works.

[ljharb]

So, if someone does eslint foo.js, will anything that's not foo.js be considered ignored?

[mdjermanovic]

So, if someone does eslint foo.js, will anything that's not foo.js be considered ignored?

No, the list of files/dirs/globs to be linted does not affect this.

On the other hand, there are CLI options that can affect this. Those are: --config, --ignore-pattern, and --no-ignore.

[ljharb]

ok - so is there any way for a rule to know that those options were provided?

(It feels more and more like this is something eslint should be abstracting away, rather than a plugin having to reconstruct eslint's logic)

[nzakas]

• --config just changes the config file being used and that becomes the config array
• --ignore-pattern is merged into the config array
• --no-ignore is also used to calculate the config array.

So isDirectoryIgnored() and isFileIgnored() takes all of this into account.

[ljharb]

ok awesome!

[nzakas]

@ljharb have you had a chance to try this out yet?

[ljharb]

no, not just yet - I'll try to make some time today or tomorrow!

[nzakas]

ping @ljharb -- we really need your feedback on this approach to help with release planning

[ljharb]

I'm trying to look into it, but since I don't use flat config anywhere yet, it's not something I can quickly test :-/

However, I feel pretty confident the approach will work, it's just unfortunate that the globbing logic has to be replicated.

[nzakas]

Is there a reason you can't just switch one of your existing repos to use flat config and try it out?

We are blocked on your feedback for this, and we are running out of time to get this into v9, so we'd appreciate you trying this out soon.

[ljharb]

Mainly because almost all of my repos are stuck on eslint v8.8.0 due to #15595, and I no longer have access to a corporate repo, so I'm not actually using the no-unused-modules rule. I'm not really sure how I can get someone to try it out prior to it being published, unfortunately.

[JoshuaKGoldberg]

I had some time to look into this. Mainly good news and a bit of bad news.

Good news: I was able to get a working no-unused-modules rule based on a limited copy & paste of FileEnumerator that receives isDirectoryIgnored and isFileIgnored instead of config logic. So I think that confirms the hope in this thread: that should be enough to support flat config (at the cost of some copy & paste). Reference PR here: import-js/eslint-plugin-import#2967.

Bad news: the specific isFileIgnored implementation in -> https://github.com/eslint/eslint/tree/issue18087-poc 4c4c1f4 seems to be giving inconsistent results for whether a file is ignored. So there's some work to be done there. I didn't have time to dig deeply yet - but @nzakas I'm guessing you have much more context than me anyway. I put a write up in https://github.com/JoshuaKGoldberg/repros/blob/0458e256d49b3887c2b5e7da27ee932d1a91ffa7/README.md.

[nzakas]

@ljharb Unfortunately, we're not going to be able to ship anything until you've verified that this approach works. The ball is in your court now and it's up to you as to whether or not this makes it in for the final v9.0.0.

@JoshuaKGoldberg My hunch is that it's in the implementation, because we use isFileIgnored() frequently in the core and have a ton of tests validating its behavior. I'd suggest instead of trying to implement a new FileEnumerator, that you just use fswalk like we do:

eslint/lib/eslint/eslint-helpers.js

Line 19 in 15c143f

const fswalk = require("@nodelib/fs.walk");

. It does have a walkSync method that should work for this purpose.

[alexrecuenco]

This might help as an example, @ljharb . I was updating the template I have for typescript-based projects

In the commit 20bfe75 I had set-up "import" in a flat config, and I was getting some errors from it, maybe some config like this might help you explore the issues?

Let me know if I can help simplify this any further.

alexrecuenco/typescript-eslint-prettier-jest-example@20bfe75