eslint: Change Request: Flat Config does not support FileEnumerator

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.

About this issue

  • Original URL
  • State: open
  • Created 5 months ago
  • Reactions: 2
  • Comments: 31 (30 by maintainers)

Most upvoted comments

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: https://github.com/import-js/eslint-plugin-import/pull/2967.

Bad news: the specific isFileIgnored implementation in -> https://github.com/eslint/eslint/tree/issue18087-poc https://github.com/eslint/eslint/commit/4c4c1f4b973b3f2039813025d047a98801c95c59 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.

@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: https://github.com/eslint/eslint/blob/15c143f96ef164943fd3d39b5ad79d9a4a40de8f/lib/eslint/eslint-helpers.js#L19. It does have a walkSync method that should work for this purpose.

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.

https://github.com/alexrecuenco/typescript-eslint-prettier-jest-example/pull/15/commits/20bfe7579ed88eb1030883a4f48dad97158b6019

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.

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

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

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.

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

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

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.