feat: Implement FlatESLint #16149
feat: Implement FlatESLint #16149
Conversation
eslint-github-bot
bot
added
triage
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Still working on getting the tests to pass. Didn't have Node.js 12 to test locally. |
|
There are some incompatibilities between the way flat config works in this PR and the way it worked when I did the original work, so I'm needing to go back through the original work and update that to get the tests to pass. |
| if (!config) { | ||
| return [ | ||
| { | ||
| ruleId: null, | ||
| severity: 1, | ||
| message: `No matching configuration found for ${filename}.`, | ||
| line: 0, | ||
| column: 0 | ||
| } | ||
| ]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Unlike with eslintrc, it's possible that the filename given might not match something in the config array. If that's the case, I've opted to create a warning saying that the file didn't have a configuration.
I wasn't sure if this should be considered an error, so I used the model of creating a warning to match what we do for ignored files in ESLint, but definitely open to other ideas.
There was a problem hiding this comment.
Deferring to precedent works for me 👍
nzakas
added
core
|
After a ferocious battle with Node.js 12 compatibility, everything is working! |
There was a problem hiding this comment.
Bravo 👏 I'm not seeing any structural issues here. Especially since this will be behind use-at-your-own-risk, I'd rather get this in and iterate than exhaustively review every corner case here. Other than the merge conflict, which I can fix myself if you don't get to it, I don't see why this shouldn't be merged before I do the release tomorrow (well, technically later today).
| * `sourceType` - The type of JavaScript source code. Possible values are `"script"` for traditional script files, `"module"` for ECMAScript modules (ESM), and `"commonjs"` for CommonJS files. (default: `"module"` for `.js` and `.mjs` files; `"commonjs"` for `.cjs` files) | ||
| * `globals` - An object specifying additional objects that should be added to the global scope during linting. | ||
| * `parser` - Either an object containing a `parse()` method or a string indicating the name of a parser inside of a plugin (i.e., `"pluginName/parserName"`). (default: `"@/espree"`) | ||
| * `parserOptions` - An object specifying additional options that are passed directly to the `parser()` method on the parser. The available options are parser-dependent. |
There was a problem hiding this comment.
Nit: typo
| * `parserOptions` - An object specifying additional options that are passed directly to the `parser()` method on the parser. The available options are parser-dependent. | |
| * `parserOptions` - An object specifying additional options that are passed directly to the `parse()` method on the parser. The available options are parser-dependent. |
|
|
||
| ### Configuring linter options | ||
|
|
||
| Options specific to the linting process can be configured using the `linterOptions` object. These effect how linting proceeds and does not affect how the source code of the file is interpreted. |
There was a problem hiding this comment.
Nit: grammar
| Options specific to the linting process can be configured using the `linterOptions` object. These effect how linting proceeds and does not affect how the source code of the file is interpreted. | |
| Options specific to the linting process can be configured using the `linterOptions` object. These affect how linting proceeds and do not affect how the source code of the file is interpreted. |
| settings: { | ||
| sharedData: "Hello" | ||
| } |
There was a problem hiding this comment.
Optional: The docs haven't covered settings yet, so seeing these here is a little unexpected. (Was this perhaps left over from a sketch related to #15949 (comment)?)
| settings: { | |
| sharedData: "Hello" | |
| } |
| settings: { | ||
| sharedData: "Hello" | ||
| } |
There was a problem hiding this comment.
Same as above
| settings: { | |
| sharedData: "Hello" | |
| } |
|
|
||
| ### Configuring rules | ||
|
|
||
| You can configure any number of rules in a configuration object by add a `rules` property containing an object with your rule configurations. The names in this object are the names of the rules and the values are the configurations for each of those rules. Here's an example: |
There was a problem hiding this comment.
Nit: verb tense
| You can configure any number of rules in a configuration object by add a `rules` property containing an object with your rule configurations. The names in this object are the names of the rules and the values are the configurations for each of those rules. Here's an example: | |
| You can configure any number of rules in a configuration object by adding a `rules` property containing an object with your rule configurations. The names in this object are the names of the rules and the values are the configurations for each of those rules. Here's an example: |
|
|
||
| const INTERNAL_FILES = { | ||
| CLI_ENGINE_PATTERN: "lib/cli-engine/**/*", | ||
| INIT_PATTERN: "lib/init/**/*", |
There was a problem hiding this comment.
Not a blocker. I can follow up with a diff later if needed. Init was removed in 0d2b9a6. Too big for an inline suggestion, but here's a diff:
diff --git eslint.config.js eslint.config.js
index 50f375d33..27bde4a02 100644
--- eslint.config.js
+++ eslint.config.js
@@ -24,7 +24,6 @@ const compat = new FlatCompat({
const INTERNAL_FILES = {
CLI_ENGINE_PATTERN: "lib/cli-engine/**/*",
- INIT_PATTERN: "lib/init/**/*",
LINTER_PATTERN: "lib/linter/**/*",
RULE_TESTER_PATTERN: "lib/rule-tester/**/*",
RULES_PATTERN: "lib/rules/**/*",
@@ -148,16 +147,6 @@ module.exports = [
rules: {
"n/no-restricted-require": ["error", [
...createInternalFilesPatterns(INTERNAL_FILES.CLI_ENGINE_PATTERN),
- resolveAbsolutePath("lib/init/index.js")
- ]]
- }
- },
- {
- files: [INTERNAL_FILES.INIT_PATTERN],
- rules: {
- "n/no-restricted-require": ["error", [
- ...createInternalFilesPatterns(INTERNAL_FILES.INIT_PATTERN),
- resolveAbsolutePath("lib/rule-tester/index.js")
]]
}
},
@@ -168,7 +157,6 @@ module.exports = [
...createInternalFilesPatterns(INTERNAL_FILES.LINTER_PATTERN),
"fs",
resolveAbsolutePath("lib/cli-engine/index.js"),
- resolveAbsolutePath("lib/init/index.js"),
resolveAbsolutePath("lib/rule-tester/index.js")
]]
}
@@ -180,7 +168,6 @@ module.exports = [
...createInternalFilesPatterns(INTERNAL_FILES.RULES_PATTERN),
"fs",
resolveAbsolutePath("lib/cli-engine/index.js"),
- resolveAbsolutePath("lib/init/index.js"),
resolveAbsolutePath("lib/linter/index.js"),
resolveAbsolutePath("lib/rule-tester/index.js"),
resolveAbsolutePath("lib/source-code/index.js")
@@ -193,7 +180,6 @@ module.exports = [
"n/no-restricted-require": ["error", [
...createInternalFilesPatterns(),
resolveAbsolutePath("lib/cli-engine/index.js"),
- resolveAbsolutePath("lib/init/index.js"),
resolveAbsolutePath("lib/linter/index.js"),
resolveAbsolutePath("lib/rule-tester/index.js"),
resolveAbsolutePath("lib/source-code/index.js")
@@ -207,7 +193,6 @@ module.exports = [
...createInternalFilesPatterns(INTERNAL_FILES.SOURCE_CODE_PATTERN),
"fs",
resolveAbsolutePath("lib/cli-engine/index.js"),
- resolveAbsolutePath("lib/init/index.js"),
resolveAbsolutePath("lib/linter/index.js"),
resolveAbsolutePath("lib/rule-tester/index.js"),
resolveAbsolutePath("lib/rules/index.js")
@@ -220,7 +205,6 @@ module.exports = [
"n/no-restricted-require": ["error", [
...createInternalFilesPatterns(INTERNAL_FILES.RULE_TESTER_PATTERN),
resolveAbsolutePath("lib/cli-engine/index.js"),
- resolveAbsolutePath("lib/init/index.js")
]]
}
}| if (!config) { | ||
| return [ | ||
| { | ||
| ruleId: null, | ||
| severity: 1, | ||
| message: `No matching configuration found for ${filename}.`, | ||
| line: 0, | ||
| column: 0 | ||
| } | ||
| ]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Deferring to precedent works for me 👍
| * @returns {Object} Contains the stats | ||
| * @private | ||
| */ | ||
| function calculateStatsPerFile(messages) { |
There was a problem hiding this comment.
I'm going to assume that these similarly-named methods here and in eslint-helpers.js are sufficiently similar to their existing implementations that I don't need to review them thoroughly at all. If any are significantly different, and you'd specifically like some eyes on them, please call them out.
| // TODO: Needed? | ||
| config = configs.getConfig(resolvedFilename); |
There was a problem hiding this comment.
I think so? Let's say we have a config like so:
import blue from 'eslint-plugin-blue';
import green from 'eslint-plugin-green';
export default [
{
files: ["blue/**/*.js"],
plugins: { color: blue }
},
{
files: ["green/**/*.js"],
plugins: { color: green }
},
{
rules: {
"color/some-rule": "error"
}
}
];Assume both plugins export a rule named some-rule and all files are either in blue/ or green/.
Is this config valid? It would depend on the order we check plugin identifier ambiguity: before or after filtering for matching config objects? If we first get the matching config objects for a file and then ensure plugin identifiers are unambiguous, this is valid because there is no ambiguity - one file will get either the blue or the green version of some-rule, but never both. In that case, we'd need the linted file's specific config to know where to load color/some-rule from to know whether it's deprecated.
| // Tests | ||
| //------------------------------------------------------------------------------ | ||
|
|
||
| describe("FlatESLint", () => { |
There was a problem hiding this comment.
I diffed this file against tests/lib/eslint/flat-eslint.js in VSCode to review. The omitted/updated/added tests all make sense, understanding that ignoring and caching are still being worked on.
|
This is behind the |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.20.0` -> `8.21.0`](https://renovatebot.com/diffs/npm/eslint/8.20.0/8.21.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.21.0`](https://github.com/eslint/eslint/releases/tag/v8.21.0) [Compare Source](eslint/eslint@v8.20.0...v8.21.0) #### Features - [`7b43ea1`](eslint/eslint@7b43ea1) feat: Implement FlatESLint ([#​16149](eslint/eslint#16149)) (Nicholas C. Zakas) - [`92bf49a`](eslint/eslint@92bf49a) feat: improve the key width calculation in `key-spacing` rule ([#​16154](eslint/eslint#16154)) (Nitin Kumar) - [`c461542`](eslint/eslint@c461542) feat: add new `allowLineSeparatedGroups` option to the `sort-keys` rule ([#​16138](eslint/eslint#16138)) (Nitin Kumar) - [`1cdcbca`](eslint/eslint@1cdcbca) feat: add deprecation warnings for legacy API in `RuleTester` ([#​16063](eslint/eslint#16063)) (Nitin Kumar) #### Bug Fixes - [`0396775`](eslint/eslint@0396775) fix: lines-around-comment apply `allowBlockStart` for switch statements ([#​16153](eslint/eslint#16153)) (Nitin Kumar) #### Documentation - [`2aadc93`](eslint/eslint@2aadc93) docs: add anchors to headings inside docs content ([#​16134](eslint/eslint#16134)) (Strek) #### Chores - [`8892511`](eslint/eslint@8892511) chore: Upgrade to Espree 9.3.3 ([#​16173](eslint/eslint#16173)) (Brandon Mills) - [`1233bee`](eslint/eslint@1233bee) chore: switch to eslint-plugin-node's maintained fork ([#​16150](eslint/eslint#16150)) (唯然) - [`97b95c0`](eslint/eslint@97b95c0) chore: upgrade puppeteer v13 ([#​16151](eslint/eslint#16151)) (唯然) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMzUuMSIsInVwZGF0ZWRJblZlciI6IjMyLjEzNS4xIn0=--> Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1483 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
eslint-github-bot
bot
added
the
archived due to age
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
This is the first implementation of
FlatESLint! 🎉Things to know:
FlatESLintimplements the flat config system. It will eventually replace the currentESLintclass completely, so long term we will have oneESLintclass that uses flat config. Just in the interim will we have two of these classes.FlatESLintcombines all of the functionality ofESLintandCLIEngine, so we will be able to deleteCLIEngineonce we switch over to the new config system for good.FlatESLintandFlatRuleTesterare exposed from theuse-at-your-own-riskentrypoint so we can get some developer feedback as we go along.FlatESLintis not accessible through the command line. This is essentially a developer-only preview and not an end-user preview.ESLint. Some of the tests no longer made sense forFlatESLint, while others didn't even make sense forESLint(there are a lot of tests that test the same things). I did my best to make sure the most important tests work.xdescribeorxitif I was blocked on fixing them, or if they relate to caching.Refs #13481
Fixes #15661
Fixes #15683
Is there anything you'd like reviewers to focus on?
Please focus on big picture issues. There will probably be some compatibility issues with the existing config system that we'll discover along the way. The goal of this PR is not to be perfect, but to land a first-pass of the new config system that we can build upon before we diverge too far away.
Also, I left a note in
linter.jsabout some of the extra checks I need to do to avoid weird errors.