Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Implement FlatESLint #16149

Merged
merged 59 commits into from Aug 1, 2022
Merged

feat: Implement FlatESLint #16149

merged 59 commits into from Aug 1, 2022

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Jul 19, 2022

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:

  1. FlatESLint implements the flat config system. It will eventually replace the current ESLint class completely, so long term we will have one ESLint class that uses flat config. Just in the interim will we have two of these classes.
  2. FlatESLint combines all of the functionality of ESLint and CLIEngine, so we will be able to delete CLIEngine once we switch over to the new config system for good.
  3. Caching is not yet implemented. That's a bit tricky and rather than holding up everything else, I'll work on that separately.
  4. FlatESLint and FlatRuleTester are exposed from the use-at-your-own-risk entrypoint so we can get some developer feedback as we go along.
  5. FlatESLint is not accessible through the command line. This is essentially a developer-only preview and not an end-user preview.
  6. There are a lot fewer tests than with ESLint. Some of the tests no longer made sense for FlatESLint, while others didn't even make sense for ESLint (there are a lot of tests that test the same things). I did my best to make sure the most important tests work.
  7. Some tests are marked out by xdescribe or xit if 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.js about some of the extra checks I need to do to avoid weird errors.

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon feature This change adds a new feature to ESLint labels Jul 19, 2022
@netlify
Copy link

netlify bot commented Jul 19, 2022

Deploy Preview for docs-eslint ready!

Name Link
馃敤 Latest commit bfe2254
馃攳 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/62e74d7ee360c0000983168f
馃槑 Deploy Preview https://deploy-preview-16149--docs-eslint.netlify.app
馃摫 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@nzakas
Copy link
Member Author

nzakas commented Jul 20, 2022

Still working on getting the tests to pass. Didn't have Node.js 12 to test locally.

@nzakas
Copy link
Member Author

nzakas commented Jul 20, 2022

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.

@nzakas nzakas marked this pull request as draft Jul 20, 2022
if (!config) {
return [
{
ruleId: null,
severity: 1,
message: `No matching configuration found for ${filename}.`,
line: 0,
column: 0
}
];
}

Copy link
Member Author

@nzakas nzakas Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@btmills btmills Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring to precedent works for me 馃憤

@nzakas nzakas marked this pull request as ready for review Jul 21, 2022
@nzakas nzakas added core Relates to ESLint's core APIs and features and removed triage An ESLint team member will look at this issue soon labels Jul 21, 2022
@nzakas
Copy link
Member Author

nzakas commented Jul 21, 2022

After a ferocious battle with Node.js 12 compatibility, everything is working!

Copy link
Member

@btmills btmills left a 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.
Copy link
Member

@btmills btmills Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: typo

Suggested change
* `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.
Copy link
Member

@btmills btmills Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: grammar

Suggested change
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"
}
Copy link
Member

@btmills btmills Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
settings: {
sharedData: "Hello"
}

settings: {
sharedData: "Hello"
}
Copy link
Member

@btmills btmills Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Suggested change
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:
Copy link
Member

@btmills btmills Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: verb tense

Suggested change
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/**/*",
Copy link
Member

@btmills btmills Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}
];
}

Copy link
Member

@btmills btmills Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring to precedent works for me 馃憤

* @returns {Object} Contains the stats
* @private
*/
function calculateStatsPerFile(messages) {
Copy link
Member

@btmills btmills Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Member

@btmills btmills Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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", () => {
Copy link
Member

@btmills btmills Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@btmills
Copy link
Member

btmills commented Aug 1, 2022

This is behind the use-at-your-own-risk export, so we can still experiment freely. Merging to include in this release!

@btmills btmills merged commit 7b43ea1 into main Aug 1, 2022
18 checks passed
@btmills btmills deleted the flat-eslint branch Aug 1, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Aug 9, 2022
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 ([#&#8203;16149](eslint/eslint#16149)) (Nicholas C. Zakas)
-   [`92bf49a`](eslint/eslint@92bf49a) feat: improve the key width calculation in `key-spacing` rule ([#&#8203;16154](eslint/eslint#16154)) (Nitin Kumar)
-   [`c461542`](eslint/eslint@c461542) feat: add new `allowLineSeparatedGroups` option to the `sort-keys` rule ([#&#8203;16138](eslint/eslint#16138)) (Nitin Kumar)
-   [`1cdcbca`](eslint/eslint@1cdcbca) feat: add deprecation warnings for legacy API in `RuleTester` ([#&#8203;16063](eslint/eslint#16063)) (Nitin Kumar)

#### Bug Fixes

-   [`0396775`](eslint/eslint@0396775) fix: lines-around-comment apply `allowBlockStart` for switch statements ([#&#8203;16153](eslint/eslint#16153)) (Nitin Kumar)

#### Documentation

-   [`2aadc93`](eslint/eslint@2aadc93) docs: add anchors to headings inside docs content ([#&#8203;16134](eslint/eslint#16134)) (Strek)

#### Chores

-   [`8892511`](eslint/eslint@8892511) chore: Upgrade to Espree 9.3.3 ([#&#8203;16173](eslint/eslint#16173)) (Brandon Mills)
-   [`1233bee`](eslint/eslint@1233bee) chore: switch to eslint-plugin-node's maintained fork ([#&#8203;16150](eslint/eslint#16150)) (鍞劧)
-   [`97b95c0`](eslint/eslint@97b95c0) chore: upgrade puppeteer v13 ([#&#8203;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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Request: Config base path in flat config Change Request: Matching behavior in flat config
2 participants