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!: Update shouldUseFlatConfig and CLI so flat config is default #17748

Merged
merged 13 commits into from Dec 20, 2023

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Nov 10, 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)

Updates shouldUseFlatConfig() to return true unless the ESLINT_USE_FLAT_CONFIG environment variable is "false".

Also ensures the CLI emits a warning when eslintrc is used.

Refs #13481

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

@nzakas nzakas requested a review from a team as a code owner November 10, 2023 16:31
@eslint-github-bot eslint-github-bot bot added breaking This change is backwards-incompatible feature This change adds a new feature to ESLint labels Nov 10, 2023
Copy link

netlify bot commented Nov 10, 2023

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 4816e25
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/658372e99d687400089a4ecc

@nzakas nzakas changed the title feat!: Update shouldUseFlatConfig so flat config is default feat!: Update shouldUseFlatConfig and CLI so flat config is default Nov 10, 2023
@nzakas
Copy link
Member Author

nzakas commented Nov 10, 2023

Tests are failing because rules are being called with syntax that isn't supported in those older Node.js versions. Need to investigate why that's happening.

@klebba
Copy link

klebba commented Nov 10, 2023

This PR timely! I was just about to open a related docs issue: https://eslint.org/docs/latest/use/configure/configuration-files-new#configuration-file-resolution

You can prevent this search for eslint.config.js by setting the ESLINT_USE_FLAT_CONFIG environment variable to true [...]

I think this should guide developers to set the variable to false

@nzakas
Copy link
Member Author

nzakas commented Nov 13, 2023

@klebba documentation updates are being tracked in #13481, and that is one of the documentation changes we'll be making.

@mdjermanovic mdjermanovic marked this pull request as draft November 15, 2023 11:21
Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

tests/bin/eslint.js Show resolved Hide resolved
tests/lib/cli.js Outdated Show resolved Hide resolved
tests/lib/cli.js Outdated Show resolved Hide resolved
tests/lib/cli.js Outdated Show resolved Hide resolved
tests/lib/cli.js Outdated
const configPath = getFixturePath(".eslintrc");
const filePath = getFixturePath("passing.js");

await cli.execute(`--config ${configPath} ${filePath}`, null, false);
Copy link
Member

Choose a reason for hiding this comment

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

After these changes, npm test will be logging more than 100 deprecation warnings until we drop eslintrc in v10.

Can we avoid that?

allowFlatConfig = false is passed only from tests, so we could update the condition in lib/cli.js to avoid logging warnings while running tests:

-if (!usingFlatConfig) {
+if (allowFlatConfig && !usingFlatConfig) {
    process.emitWarning("You are using an eslintrc configuration file, which is deprecated and support will be removed in v10.0.0. Please migrate to an eslint.config.js file. See https://eslint.org/docs/latest/use/configure/migration-guide for details.", "ESLintRCWarning");
}

Then, we could update this test this way to make it work:

Suggested change
await cli.execute(`--config ${configPath} ${filePath}`, null, false);
const localCLI = proxyquire("../../lib/cli", {
"./eslint/flat-eslint": { shouldUseFlatConfig: () => Promise.resolve(false) }
});
await localCLI.execute(`--config ${configPath} ${filePath}`);

Or, perhaps even better, merge this test into the test for ESLINT_USE_FLAT_CONFIG=false as that's anyway the most realistic test:

it(`should not use it when ESLINT_USE_FLAT_CONFIG=false even if an eslint.config.js is present:${configType}`, async () => {
    process.env.ESLINT_USE_FLAT_CONFIG = "false";
    process.cwd = getFixturePath;

    const exitCode = await cli.execute(`--no-ignore --ext .js ${getFixturePath("files")}`, null, useFlatConfig);

    assert.strictEqual(exitCode, 0);

+   if (useFlatConfig) {
+       assert.strictEqual(processStub.callCount, 1, "calls `process.emitWarning()` once");
+       assert.strictEqual(processStub.getCall(0).args[1], "ESLintRCWarning");
+   }
});

That would leave npm test with zero eslintrc deprecation warnings (assuming #17797 will be merged; those tests pass allowFlatConfig = true but mistakenly still work in eslintc mode, so they're mistakenly logging deprecation warnings).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion, I went with the second approach.

lib/cli.js Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Dec 7, 2023

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Dec 7, 2023
@mdjermanovic mdjermanovic added cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion labels Dec 14, 2023
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!

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.

This might fix merge conflicts.

tests/lib/cli.js Outdated Show resolved Hide resolved
nzakas and others added 7 commits December 20, 2023 13:50
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@nzakas nzakas marked this pull request as ready for review December 20, 2023 18:50
Copy link

linux-foundation-easycla bot commented Dec 20, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@nzakas
Copy link
Member Author

nzakas commented Dec 20, 2023

Only CLA issues are with commits from CI, so this should be safe to merge.

@nzakas nzakas merged commit 8fe8c56 into main Dec 20, 2023
17 checks passed
@nzakas nzakas deleted the should-use-flat-config branch December 20, 2023 23:20
nzakas added a commit that referenced this pull request Dec 20, 2023
@nzakas nzakas restored the should-use-flat-config branch December 20, 2023 23:31
@nzakas
Copy link
Member Author

nzakas commented Dec 20, 2023

Will need to revisit once #17889 is merged

bmish added a commit to bmish/eslint that referenced this pull request Dec 27, 2023
* main: (25 commits)
  test: ensure that CLI tests run with FlatESLint (eslint#17884)
  fix!: Behavior of CLI when no arguments are passed (eslint#17644)
  docs: Update README
  Revert "feat!: Remove CodePath#currentSegments" (eslint#17890)
  feat!: Update shouldUseFlatConfig and CLI so flat config is default (eslint#17748)
  feat!: Remove CodePath#currentSegments (eslint#17756)
  chore: update dependency markdownlint-cli to ^0.38.0 (eslint#17865)
  feat!: deprecate no-new-symbol, recommend no-new-native-nonconstructor (eslint#17710)
  feat!: check for parsing errors in suggestion fixes (eslint#16639)
  feat!: assert suggestion messages are unique in rule testers (eslint#17532)
  feat!: `no-invalid-regexp` make allowConstructorFlags case-sensitive (eslint#17533)
  fix!: no-sequences rule schema correction (eslint#17878)
  feat!: Update `eslint:recommended` configuration (eslint#17716)
  feat!: drop support for string configurations in flat config array (eslint#17717)
  feat!: Remove `SourceCode#getComments()` (eslint#17715)
  feat!: Remove deprecated context methods (eslint#17698)
  feat!: Swap FlatESLint-ESLint, FlatRuleTester-RuleTester in API (eslint#17823)
  feat!: remove formatters except html, json(-with-metadata), and stylish (eslint#17531)
  feat!: Require Node.js `^18.18.0 || ^20.9.0 || >=21.1.0` (eslint#17725)
  fix: allow circular references in config (eslint#17752)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

4 participants