[New] add support for Flat Config

[michaelfaith]

This change adds support for ESLint's new Flat config system. It maintains backwards compatibility with eslintrc style configs as well.

To achieve this, we're now dynamically creating flat configs on a new flatConfigs export.

Example Usage

import importPlugin from 'eslint-plugin-import';
import js from '@eslint/js';
import tsParser from '@typescript-eslint/parser';

export default [
  js.configs.recommended,
  importPlugin.flatConfigs.recommended,
  importPlugin.flatConfigs.react,
  importPlugin.flatConfigs.typescript,
  {
    files: ['**/*.{js,mjs,cjs,jsx,mjsx,ts,tsx,mtsx}'],
    languageOptions: {
      parser: tsParser,
      ecmaVersion: 'latest',
      sourceType: 'module',
    },
    ignores: ['eslint.config.js'],
    rules: {
      'no-unused-vars': 'off',
      'import/no-dynamic-require': 'warn',
      'import/no-nodejs-modules': 'warn',
    },
  },
];

I wasn't able to reproduce any issues with the parser (as mentioned in #2556), so there's nothing here specifically to address that. And just to be clear, this is only aiming to provide support for flat config (which is a separate issue than supporting eslint v9), and has only been tested with the latest version of v8. I created two testbeds under a new examples folder, one for legacy and one for flat, each setup with the @typescript-eslint/parser and a handful of ts and tsx files that contain rule violations, to ensure the parsing still works as expected. I also checked that the parsing workflow is happening properly by scattering some log messages at different points in the logic that resolves the parser, and in both legacy and flat setups, it's getting the parser ok (screenshots below). It looks like there was already code to navigate the fact that the parsing options have changed shape in the new config format. So if @TomerAberbach or anyone else that's had issues with the plugin parsing can test this branch out with their use case or give guidance on how to reproduce their issue, that could help. Otherwise, i think this should satisfy both legacy and flat configs.

I do think there should be a larger refactor at some point to move away from the parser by name paradigm and embrace the new way of passing a parser object, but it wasn't necessary to do that here. Maybe something to consider for v9 support (or v10, since some of the deprecated functions will be removed in v10).

Legacy Config Execution:

Flat Config Execution:

Closes #2556

[codecov]

Codecov Report

Attention: Patch coverage is 61.72840% with 31 lines in your changes missing coverage. Please review.

Project coverage is 95.07%. Comparing base (09476d7) to head (e4ae179).

Files	Patch %	Lines
src/rules/no-unused-modules.js	68.85%	19 Missing ⚠️
src/core/fsWalk.js	7.69%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3018      +/-   ##
==========================================
- Coverage   96.02%   95.07%   -0.96%     
==========================================
  Files          78       80       +2     
  Lines        3299     3349      +50     
  Branches     1160     1182      +22     
==========================================
+ Hits         3168     3184      +16     
- Misses        131      165      +34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[michaelfaith]

@ljharb I found this issue you logged about the no-unused-modules rule specifically eslint/eslint#18087. So it may not be possible to land this until that has been addressed. But maybe this can branch can help provide a testing ground to implement that? It seemed one of the issues you were having was a good place to test the approach.

[michaelfaith]

@ljharb It appears that the no-unused-modules rule is still working in 8.x with flat config. I added that to both of the example tests. I also added several console logs to the rule to see if all of the files were being populated correctly, and it all seems to behave the same as the legacy config. For v9+ I think @nzakas' proposed api change (eslint/eslint#18087 (comment)) along with his recommendation of using @nodelib/fs-walk for walking the files under cwd (walkSync) would work.

Rule Execution:

tl;dr: i think this could be merged and released for 8.x flat config support, unless there's an additional element i'm not considering.

[ljharb]

@michaelfaith if we can run the tests in flat config also, and they pass, then that's good enough for me - it's probably a good idea to implement the proposed eslint API (as a fallback prior to whenever eslint ships it) and base it on that, then we can use the built-in API once it's available?

[michaelfaith]

it's probably a good idea to implement the proposed eslint API (as a fallback prior to whenever eslint ships it) and base it on that, then we can use the built-in API once it's available?

Makes sense. Would you like that part of this change, or a separate PR?

[ljharb]

Seems like part of this PR if it’s only going to be used in flat config

[controversial]

On the other hand, there might be some value in merging support for v8 flat config first, if it’s ready and tested, and then releasing eslint v9 support separately

[ljharb]

@controversial thats what this PR should be doing.

[controversial]

My mistake, I had misremembered and thought the “proposed eslint API” from eslint/eslint#18087 was to cover a v9-removed API, rather than a flat config incompatibility

[nzakas]

it's probably a good idea to implement the proposed eslint API (as a fallback prior to whenever eslint ships it) and base it on that, then we can use the built-in API once it's available?

Just want to flag that there's no guarantee that the API I proposed and prototyped will ship. It still needs to be RFCed based on feedback. It was just an idea to test out feasibility.

[michaelfaith]

it's probably a good idea to implement the proposed eslint API (as a fallback prior to whenever eslint ships it) and base it on that, then we can use the built-in API once it's available?

Just want to flag that there's no guarantee that the API I proposed and prototyped will ship. It still needs to be RFCed based on feedback. It was just an idea to test out feasibility.

@nzakas In that sense, is there anything else you need from this project to move forward with the proposed api updates? Or is the runway clear?

[nzakas]

If you can comment back on eslint/eslint#18087 with the results of using the prototype that would help. It would also help to know if you're accessing files that ESLint might not have linted during its lifecycle. (For example, if I run eslint foo.js, are you only ever looking at foo.js or are you still looking at all the files?)

[ljharb]

@nzakas we're still looking at all the files - at a minimum, all the files that foo.js directly or transitively imports.

[michaelfaith]

If you can comment back on eslint/eslint#18087 with the results of using the prototype that would help.

Was that poc branch published to a pre-release version? Or what's the best way to consume that POC here?

[nzakas]

@ljharb hmmm okay, then this probably won't work without async rules, which are a ways off.

@michaelfaith you'll need to check out the branch mentioned in the issue.

[ljharb]

@nzakas sorry if that wasn't made clear that that's how we're using FileEnumerator - basically we traverse every lintable file and build up a complete dependency graph, and then go from there.

[michaelfaith]

My mistake, I had misremembered and thought the “proposed eslint API” from eslint/eslint#18087 was to cover a v9-removed API, rather than a flat config incompatibility

@controversial I don't think this is actually the case. I'm seeing the flat config without this additional change working just fine in v8 with the changes I've already made. I don't really have a horse in the race as far as whether this should go in with or without the additional changes we've discussed, and am happy to do it either way, but purely from a v8 flat-config compatibility perspective, I believe this could be released as is. The rule in question no-unused-modules is still working with the og FileEnumerator in the flat config example I added.

[controversial]

If this PR includes thorough tests of everything working under eslint v8 flat config, then I don’t see a reason to avoid releasing it!

[ljharb]

Indeed, if the FileEnumerator problem only applies in v9 and not in flat config, then it'd be fine - but that wasn't my understanding of the problem. In flat config, does FileEnumerator still respect the eslint config's ignore settings, for example?

[michaelfaith]

In flat config, does FileEnumerator still respect the eslint config's ignore settings, for example?

In my local, I added log messages in several places to see how FileEnumerator behaved, specifically for the no-unused-modules rule.

Here is the list of files obtained under different scenarios (in all cases exports.ts has a violation of the no-unused-modules rule):

legacy rc without ignoring **/exports.ts

legacy rc ignoring **/exports.ts

flat config without ignoring **/exports.ts

flat config ignoring **/exports.ts

So, you're right that the files being processed by the rule include files that were ignored, which means the rule is having to do more work than it should (not good from a performance perspective). Though, interestingly they're not being reported as violations (i.e. not having any obvious user-facing impact). I'm happy to keep working on this in this PR, but with the violations still reporting as expected, not sure how you want to treat it.

[michaelfaith]

I added another exports-unused.ts to both sets of examples, with each config ignoring the file. That way exports.ts can still demonstrate the rule violation working, while we explore the ignored files difference.

[nzakas]

Okay, a little bit of history here to help clear things up. :)

FileEnumerator was an internal-only API that we used for finding files inside of ESLint based on the eslintrc configuration system. It takes care of reading in every configuration file and figuring out which files should be returned for linting.

This plugin was only able to use FileEnumerator because early on Node.js allowed access to any file in the package whether or not it was mentioned in package.json. Once we decided to lock down the API to prevent this from happening, we agreed to leave FileEnumerator exposed so as not to break eslint-plugin-import in the short term. But we did warn that this wasn't going to be forever and that we didn't plan on creating a replacement for FileEnumerator either internally or externally.

All that is to say, FileEnumerator does not read flat config files and therefore can't be used effectively when the project uses flat config files.

This use case (crawling into files that aren't part of the lint session) isn't something that ESLint can formally support at this point, so it will require some trickery or imperfect solutions if it's going to work at all.

To that end, though, it may be worth considering pointing people to Knip, which is its own standalone tool that can solve this same problem without the constraints of running inside of ESLint.

[michaelfaith]

To that end, though, it may be worth considering pointing people to Knip, which is its own standalone tool that can solve this same problem without the constraints of running inside of ESLint.

The implication being that this plugin phases out the rule entirely? Seems reasonable, actually. I.e. use the right tool for the job, rather than trying to force eslint to do something it's not intended for.

[guillaumebrunerie]

This use case (crawling into files that aren't part of the lint session) isn't something that ESLint can formally support at this point

Isn’t it reasonable to expect being able to use ESLint to find unused exports? Unused exports are a potential problem, in exactly the same way as unused variables are: "most likely an error due to incomplete refactoring. Such [exports] take up space in the code and can lead to confusion by readers". And finding such potential issues is the whole point of ESLint. I was actually very surprised when I first used ESLint to learn that this rule is not part of ESLint core but is only available via some third-party plugin.

[ljharb]

It's quite reasonable - however we could certainly use knip or something similar inside the rule as an alternative way to find unused files, if it's compatible with the rule. (i used knip as the core of https://www.npmjs.com/package/@ljharb/unused-files, so I'm aware it probably won't work in this project, but that's a possible direction to go)

@nzakas why would requireability matter for eslint? I assumed you'd use fs functions, which aren't constrained by the exports field.

[michaelfaith]

Even if I were to re-implement what fs.walk's sync function is doing, which I wouldn't mind doing, I'd still want to use fs apis that were introduced in node v10 (e.g. Dirent). I'm not super interested in working with node v4 APIs. That seems like an unnecessary constraint. Happy to hand this off to someone else, if that remains the expectation. I also think this PR can land without addressing the no-unused-module rule issue, if pull the last commit back out. The flat config support is working without it

[mshima]

Even if I were to re-implement what fs.walk's sync function is doing, which I wouldn't mind doing, I'd still want to use fs apis that were introduced in node v10 (e.g. Dirent).

It’s possible to use FileEnumerator for ESLint < v9 and fall back to lazy load fs api for eslint v9 which requires node >= 18.18.0.
Lazy loading will allow new apis to be used.

[mshima]

In current PR if @nodelib/fs.walk is lazy loaded I think CI will pass.
The missing task is to replace @nodelib/fs.walk with in module implementation.

[controversial]

@mshima - @ljharb stated that he’s not willing to approve fs.walk as a dependency, given its engines declaration.

But if @michaelfaith is willing to rebuild part of fs.walk functionality, I imagine that using node 10+ APIs there would be fine, since those could be lazy-loaded at runtime, for flat config users only (who will have compatible node versions as dictated by their eslint version), without adding any dependencies

[michaelfaith]

Seems like a reasonable approach. I'll go that route

[ljharb]

That’s exactly right; conditionally using modern APIs is perfectly fine; an engines declaration that would break everyone is not.

[michaelfaith]

@ljharb I implemented a local version of walkSync (src/core/fsWalk.js) and removed the dependency on @nodelib/fsWalk. I made it in such a way that when / if this library ever does do a major version and is ok with installing the full lib, it'd just be an import path swap in the rule.
So, I think I've addressed everything, if you want to give it another once over.

[controversial]

That’s amazing @michaelfaith; thanks for all your hard work

[mohammedhammoud]

Great job on this! ⭐ Do you have any idea when it will be released?

[ljharb]

Thanks!

(Rebased, squashed, removed the "stage 0" flat config that nobody should be using anyways, reverted some of the prettier mess in the rule, fixed a bug in the rule in resolveFiles, and cleaned up fsWalk a bit)

[controversial]

@nzakas It looks like this PR was just merged with an implementation that includes support for context.session.isFileIgnored and context.session.isDirectoryIgnored!

There’s still a fallback to use FileEnumerator where these APIs aren’t available, but it seems like the proposed APIs from eslint/eslint#18087 are ready to be used by eslint-plugin-import if/when eslint is able to ship them (see listFilesWithModernApi). Of course, this would also depend on eslint-plugin-import gaining support for eslint 9.x, assuming that the context.session.is*Ignored APIs wouldn’t be backported to a new 8.x release.

[michaelfaith]

Thanks!

(Rebased, squashed, removed the "stage 0" flat config that nobody should be using anyways, reverted some of the prettier mess in the rule, fixed a bug in the rule in resolveFiles, and cleaned up fsWalk a bit)

Thanks for your attention on this.