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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: Switch Linter to flat config by default #17851

Merged
merged 4 commits into from Dec 24, 2023
Merged

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Dec 13, 2023

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)

Made flat config mode the default for Linter and updated associated code.

Refs #13481

Is there anything you'd like reviewers to focus on?

@eslint-github-bot eslint-github-bot bot added breaking This change is backwards-incompatible feature This change adds a new feature to ESLint labels Dec 13, 2023
Copy link

netlify bot commented Dec 13, 2023

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 667fb02
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6585db6320ff820008640230

@@ -23,7 +23,7 @@ describe("eslint-fuzzer", function() {
*/
this.timeout(15000); // eslint-disable-line no-invalid-this -- Mocha timeout

const linter = new eslint.Linter();
const linter = new eslint.Linter({ configType: "eslintrc" });
Copy link
Member Author

Choose a reason for hiding this comment

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

When we remove eslintrc mode, we'll need to revisit how the fuzzer works.

@@ -17505,32 +17505,6 @@ var a = "test2";
});
});

describe("Mutability", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why these are still here. There's no way to test this using flat config.

@@ -498,7 +498,7 @@ class RuleTester {
* @type {Object}
*/
this.rules = {};
this.linter = new Linter();
this.linter = new Linter({ configType: "eslintrc" });
Copy link
Member Author

Choose a reason for hiding this comment

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

Temporary measure until we remove RuleTester

@nzakas nzakas marked this pull request as ready for review December 20, 2023 23:16
@nzakas nzakas requested a review from a team as a code owner December 20, 2023 23:16
*/
constructor({ cwd, configType } = {}) {
constructor({ cwd, configType = "flat" } = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

When config argument is not passed to verify/verifyAndFix, Linter still runs eslintrc code paths, which is probably not what we want if "flat" is the new default.

An observable difference:

const { Linter } = require("eslint");

const linter = new Linter();

const results = linter.verify(`

    /* eslint no-undef: 2 */
    /* eslint-env browser */

    window;

`);

console.log(results); // [], but there should be a no-undef lint message in flat config mode

The cause is the condition it uses to switch between modes:

eslint/lib/linter/linter.js

Lines 1371 to 1372 in a9a17b3

if (config) {
if (configType === "flat") {

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, in that case adding a default value to the constructor won't fix the problem, because we are still checking for config. Probably need to revisit all of that logic.

@nzakas
Copy link
Member Author

nzakas commented Dec 22, 2023

I changed the condition in verify(). 👍

assert.throws(() => {
linter.verify("foo", { rules: { "test-rule": "error" } });
}, TypeError, "Could not find \"test-rule\" in plugin \"@\".");

Copy link
Member

Choose a reason for hiding this comment

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

I believe this is a test file for the linter/rules.js module, which is only used in eslintrc mode so it might make more sense to keep the tests in eslintrc mode. However, I'm not sure why these particular tests were in this file since they relate to functionalities implemented in the Linter class, so seems best to revisit this at some later point.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 2916c63 into main Dec 24, 2023
17 checks passed
@mdjermanovic mdjermanovic deleted the flat-config-linter branch December 24, 2023 14:37
bmish added a commit to bmish/eslint that referenced this pull request Dec 27, 2023
* main:
  docs: Migrate to v9.0.0 (eslint#17905)
  docs: Switch to flat config by default (eslint#17840)
  docs: Update Create a Plugin for flat config (eslint#17826)
  feat!: add two more cases to `no-implicit-coercion` (eslint#17832)
  docs: Switch shareable config docs to use flat config (eslint#17827)
  chore: remove creating an unused instance of Linter in tests (eslint#17902)
  feat!: Switch Linter to flat config by default (eslint#17851)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change is backwards-incompatible feature This change adds a new feature to ESLint
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

2 participants