[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[bot]]

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 :warning:
src/core/fsWalk.js	7.69%	12 Missing :warning:

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[michaelfaith]

@ljharb I found this issue you logged about the no-unused-modules rule specifically https://github.com/eslint/eslint/issues/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 (https://github.com/eslint/eslint/issues/18087#issuecomment-1944319008) 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 https://github.com/eslint/eslint/issues/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 https://github.com/eslint/eslint/issues/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]

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.

Correct me if I'm wrong, but I think it has more to do with the paradigm of ESLint being that rules should be something that can be validated within a single file. Reaching across multiple files in order to validate a rule, breaks that paradigm. It's not to say that it isn't a worthy thing to care about, but just that there are other tools that are more suited for that purpose. At least that's what I'm reading into it.

[nzakas]

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

@ljharb Sorry, I lost the thread and can't find what this is in relation to. Can you expand?

Correct me if I'm wrong, but I think it has more to do with the paradigm of ESLint being that rules should be something that can be validated within a single file. Reaching across multiple files in order to validate a rule, breaks that paradigm

@guillaumebrunerie @michaelfaith This is correct. The way ESLint works is on one rule at a time, and there is no concept of information from one file being available in another file. Whether or not this should be added is a good discussion to have, but it's a nontrivial architectural change that would require a lot of thought and planning. I think in an ideal world, ESLint is capable of doing this in the core rather than forcing rules to work through this on their own. It's something I've pondered for a bit, and something that the core rewrite will give us an opportunity to explore.

[ljharb]

@nzakas

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.

node continues to allow that via fs.readFile and friends; it's only the exports field that requires require or import of files, so I wouldn't expect that the exports field has any impact on linting.

[nzakas]

Gotcha. Minor point - just don't use internal APIs. :)

[ljharb]

ahhh you’re saying that’s why we were able to access FileEnumerator - yes, that’s true :-)

[socket-security[bot]]

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@eslint/js@9.9.1	None	0	14.2 kB	eslintbot
npm/@types/node@20.16.2	None	0	2.16 MB	types
npm/@typescript-eslint/parser@7.18.0	Transitive: environment, filesystem	+22	3.16 MB	jameshenry
npm/cross-env@7.0.3	environment	0	29.1 kB	kentcdodds
npm/typescript@5.5.4	None	0	21.9 MB	typescript-bot

View full report↗︎

[michaelfaith]

@ljharb I had some time to put towards this refactor, using the tentative apis that @nzakas introduced in his branch. Everything looks good with those new functions. I hooked up the the examples to the local build of that branch, and verified that it gathered the list of files correctly, using walkSync as @nzakas suggested. The one caveat I noticed is that, while ignores were working as expected for most files, it it was not ignoring the eslint.config.mjs file itself, despite that being listed in the ignoreFiles array.

vs

The other ignore patterns worked fine. It just seems to be including the config regardless. That's more feedback for Nicholas on the proposed API, than it is a blocker for this PR.

[michaelfaith]

Also, worth mentioning, when running with the 9.0.0-beta branch, there were issued with other rules that will need to be resolved as part of the v9 update. Namely, the importDeclaration function includes a call to context.getAncestors() which doesn't seem to exist anymore. But that shouldn't hold up this PR since it's just focused on flat config support.

export default function importDeclaration(context) {
  const ancestors = context.getAncestors();
  return ancestors[ancestors.length - 1];
}

[michaelfaith]

Oh, and @nodelib/fs.walk has engine declaration of node >=16.14.0.

[ljharb]

Oh, and @nodelib/fs.walk has engine declaration of node >=16.14.0.

then we can't use it, unfortunately.

[controversial]

Note that eslint@9 requires Node.js ^18.18.0 || ^20.9.0 || >=21.1.0

So if eslint-plugin-import were willing to skip supporting eslint@8 flat config (i.e., declare that flat config will only be supported for eslint>=9) then @nodelib/fs.walk could be used without breaking compatibility for anyone:

• For eslint<9 users, this plugin could use FileEnumerator under the old config format
• For eslint>=9 users (who all have flat configs), this plugin could use @nodelib/fs.walk because those users necessarily have a compatible node version.

[controversial]

In fact, I don’t think it’s clear whether the (currently still hypothetical) eslint API from https://github.com/eslint/eslint/issues/18087 is being considered at all for a future 8.x release, or whether it’s only being considered to add to a future 9.x ?

If it’s the latter, then the @nodelib/fs.walk solution wouldn’t be viable for any 8.x users anyway.

[ljharb]

@controversial that would work perfectly fine, except that engines.node declarations aren't conditional.

[controversial]

What about optionalDependencies?

1. eslint-plugin-import adds @nodelib/fs.walk to optionalDependencies
2. We assume that it will be installed for any user on eslint@9 (because otherwise the eslint@9 engines declaration will have failed)
3. If this is not the case at runtime (ie @nodelib/fs.walk cannot be found for a user on eslint@9) throw an exception due to invariant violation - it should have been installed if the user is on a supported engine, which they should be if they’re using eslint@9

[silverwind]

engines.node declarations aren't conditional.

engines.node ultimately does not matter, it's just an advice. You can put anything you want in there, package managers will still install the module just fine. I recommend you put the lowest supported version. Anyone running eslint 9 will be using a supported node version anyways because eslint would likely crash before this plugin when ran on a unsupported version.

[ljharb]

@silverwind that's not accurate. It will cause npm ls to fail, it will fail installs that use engines.strict, and in an upcoming version of npm, it will cause that version of a package to not be installed at all.

The issue is that @nodelib/fs.walk has engines.node set to '>=16.14.0'.

[silverwind]

@silverwind that's not accurate. It will cause npm ls to fail, it will fail installs that use engines.strict, and in an upcoming version of npm, it will cause that version of a package to not be installed at all.

The issue is that @nodelib/fs.walk has engines.node set to '>=16.14.0'.

I doubt npm will ever go that route. But if this is really a concern, puttting it in optionalDependencies as suggested is the way to go.

[controversial]

in an upcoming version of npm, it will cause that version of a package to not be installed at all

Any “upcoming version of npm” requires at least node 18, so we don’t need to worry about whether a future npm version might create problems for node 16.x users

[ljharb]

npm's already decided to go that route (in that it'll autoselect a version of the package whose engines declarations match).

either way, adding an optionalDep is a breaking change.

To be clear, I'm not going to add any transitive engines declarations to the runtime deps of any package, that conflict with the package's own engines declarations. That's a nonstarter.

[silverwind]

Maybe just copy any of the existing directory walking implementations then, there are hundrets of these on npm, I maintain one myself too but it's node >= 18, so unsuitable.

[ljharb]

Sure, that's fine if that's the best available option.

[controversial]

Why is adding an optionalDep a breaking change?

[ljharb]

@controversial because if someone already has it installed, but the version doesn't satisfy the added range, the dep graph will no longer be valid (install may fail, and ls will certainly fail).

"Optional" means if it fails to compile it's ok, but it doesn't mean that it's ok if the dep exists at an incompatible version.

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