-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[New] add support for Flat Config #3018
Conversation
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. Usage ```js 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', }, }, ]; ```
Codecov ReportAttention: Patch coverage is
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. |
7aadb19
to
e1a5fda
Compare
@ljharb I found this issue you logged about the |
e1a5fda
to
26fae9f
Compare
@ljharb It appears that the 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. |
@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? |
Makes sense. Would you like that part of this change, or a separate PR? |
Seems like part of this PR if it’s only going to be used in flat config |
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 |
@controversial thats what this PR should be doing. |
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 |
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? |
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 |
@nzakas we're still looking at all the files - at a minimum, all the files that |
Was that poc branch published to a pre-release version? Or what's the best way to consume that POC here? |
@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. |
@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. |
@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 |
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! |
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? |
In my local, I added log messages in several places to see how Here is the list of files obtained under different scenarios (in all cases legacy rc without ignoring
|
26fae9f
to
c89ab60
Compare
I added another |
Okay, a little bit of history here to help clear things up. :)
This plugin was only able to use All that is to say, 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. |
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. |
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. |
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 |
Even if I were to re-implement what |
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. |
In current PR if |
@mshima - @ljharb stated that he’s not willing to approve But if @michaelfaith is willing to rebuild part of |
Seems like a reasonable approach. I'll go that route |
That’s exactly right; conditionally using modern APIs is perfectly fine; an engines declaration that would break everyone is not. |
@ljharb I implemented a local version of |
f02ff85
to
e4ae179
Compare
That’s amazing @michaelfaith; thanks for all your hard work |
Great job on this! ⭐ Do you have any idea when it will be released? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
e4ae179
to
806e3c2
Compare
@nzakas It looks like this PR was just merged with an implementation that includes support for 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 |
Thanks for your attention on this. |
| datasource | package | from | to | | ---------- | -------------------- | ------ | ------ | | npm | eslint-plugin-import | 2.29.1 | 2.30.0 | ## [v2.30.0](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#2300---2024-09-02) ##### Added - \[`dynamic-import-chunkname`]: add `allowEmpty` option to allow empty leading comments (\[[#2942](import-js/eslint-plugin-import#2942)], thanks \[[@JiangWeixian](https://github.com/JiangWeixian)]) - \[`dynamic-import-chunkname`]: Allow empty chunk name when webpackMode: 'eager' is set; add suggestions to remove name in eager mode (\[[#3004](import-js/eslint-plugin-import#3004)], thanks \[[@amsardesai](https://github.com/amsardesai)]) - \[`no-unused-modules`]: Add `ignoreUnusedTypeExports` option (\[[#3011](import-js/eslint-plugin-import#3011)], thanks \[[@silverwind](https://github.com/silverwind)]) - add support for Flat Config (\[[#3018](import-js/eslint-plugin-import#3018)], thanks \[[@michaelfaith](https://github.com/michaelfaith)]) ##### Fixed - \[`no-extraneous-dependencies`]: allow wrong path (\[[#3012](import-js/eslint-plugin-import#3012)], thanks \[[@chabb](https://github.com/chabb)]) - \[`no-cycle`]: use scc algorithm to optimize (\[[#2998](import-js/eslint-plugin-import#2998)], thanks \[[@soryy708](https://github.com/soryy708)]) - \[`no-duplicates`]: Removing duplicates breaks in TypeScript (\[[#3033](import-js/eslint-plugin-import#3033)], thanks \[[@yesl-kim](https://github.com/yesl-kim)]) - \[`newline-after-import`]: fix considerComments option when require (\[[#2952](import-js/eslint-plugin-import#2952)], thanks \[[@developer-bandi](https://github.com/developer-bandi)]) - \[`order`]: do not compare first path segment for relative paths (\[[#2682](import-js/eslint-plugin-import#2682)]) (\[[#2885](import-js/eslint-plugin-import#2885)], thanks \[[@mihkeleidast](https://github.com/mihkeleidast)]) ##### Changed - \[Docs] `no-extraneous-dependencies`: Make glob pattern description more explicit (\[[#2944](import-js/eslint-plugin-import#2944)], thanks \[[@mulztob](https://github.com/mulztob)]) - \[`no-unused-modules`]: add console message to help debug \[[#2866](import-js/eslint-plugin-import#2866)] - \[Refactor] `ExportMap`: make procedures static instead of monkeypatching exportmap (\[[#2982](import-js/eslint-plugin-import#2982)], thanks \[[@soryy708](https://github.com/soryy708)]) - \[Refactor] `ExportMap`: separate ExportMap instance from its builder logic (\[[#2985](import-js/eslint-plugin-import#2985)], thanks \[[@soryy708](https://github.com/soryy708)]) - \[Docs] `order`: Add a quick note on how unbound imports and --fix (\[[#2640](import-js/eslint-plugin-import#2640)], thanks \[[@MinervaBot](https://github.com/minervabot)]) - \[Tests] appveyor -> GHA (run tests on Windows in both pwsh and WSL + Ubuntu) (\[[#2987](import-js/eslint-plugin-import#2987)], thanks \[[@joeyguerra](https://github.com/joeyguerra)]) - \[actions] migrate OSX tests to GHA (\[[ljharb#37](ljharb/eslint-plugin-import#37)], thanks \[[@aks-](https://github.com/aks-)]) - \[Refactor] `exportMapBuilder`: avoid hoisting (\[[#2989](import-js/eslint-plugin-import#2989)], thanks \[[@soryy708](https://github.com/soryy708)]) - \[Refactor] `ExportMap`: extract "builder" logic to separate files (\[[#2991](import-js/eslint-plugin-import#2991)], thanks \[[@soryy708](https://github.com/soryy708)]) - \[Docs] \[`order`]: update the description of the `pathGroupsExcludedImportTypes` option (\[[#3036](import-js/eslint-plugin-import#3036)], thanks \[[@liby](https://github.com/liby)]) - \[readme] Clarify how to install the plugin (\[[#2993](import-js/eslint-plugin-import#2993)], thanks \[[@jwbth](https://github.com/jwbth)])
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
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 forlegacy
and one forflat
, each setup with the@typescript-eslint/parser
and a handful ofts
andtsx
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:
![legacy](https://private-user-images.githubusercontent.com/8071845/341159020-e42f2006-e087-4573-a73d-5b91360b344f.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU1OTI1MzYsIm5iZiI6MTcyNTU5MjIzNiwicGF0aCI6Ii84MDcxODQ1LzM0MTE1OTAyMC1lNDJmMjAwNi1lMDg3LTQ1NzMtYTczZC01YjkxMzYwYjM0NGYucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDkwNiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA5MDZUMDMxMDM2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9N2RhMTA5ZDI0OTlhYWJjMWViNmM5NDg2MDMyZjk1ZDA3MGI3MjNlYWZhMTAxNzAxNGM1NThmNjAxMTI4MmQ2YyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.ZPSXTPrnYL0ialqCVFJIvAisqMgcksgdjf1uZgjCslY)
Flat Config Execution:
![flat](https://private-user-images.githubusercontent.com/8071845/341159093-e963a5b8-0eea-48d0-83af-f5db1e03ae79.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU1OTI1MzYsIm5iZiI6MTcyNTU5MjIzNiwicGF0aCI6Ii84MDcxODQ1LzM0MTE1OTA5My1lOTYzYTViOC0wZWVhLTQ4ZDAtODNhZi1mNWRiMWUwM2FlNzkucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDkwNiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA5MDZUMDMxMDM2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ODY4ZWYyZTRhYmE5NTI1YjAzYzFiNGYxOTFiYjQzM2I3NTJhMWIwNzk1ZmVmNjBkYjNhNTEzODFhNWIxOTRlNSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.9DAodEPiDpkWXKJ2VSKZBLbDLR8XhgZNLaztkDvpx-c)
Closes #2556