Skip to content

fix: Throw error when invalid flags passed#18705

Merged
mdjermanovic merged 5 commits intomainfrom
feature-flags-docs
Jul 23, 2024
Merged

fix: Throw error when invalid flags passed#18705
mdjermanovic merged 5 commits intomainfrom
feature-flags-docs

Conversation

@nzakas
Copy link
Copy Markdown
Member

@nzakas nzakas commented Jul 19, 2024

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[x] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

  • Linter now validates flags and throws an error for any inactive or unknown flags. Previously, these flags were ignored, which made it difficult to track down problems.
  • Added a new "Feature Flags" page that dynamically populates the available feature flags and shows how to use them.

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

@nzakas nzakas requested a review from a team as a code owner July 19, 2024 21:26
@eslint-github-bot eslint-github-bot Bot added the bug ESLint is working incorrectly label Jul 19, 2024
@github-actions github-actions Bot added the core Relates to ESLint's core APIs and features label Jul 19, 2024
@netlify
Copy link
Copy Markdown

netlify Bot commented Jul 19, 2024

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit e3dfcd3
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/669fc316f8a9910008a06b8c

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jul 20, 2024
Comment thread lib/linter/linter.js
Comment on lines +1286 to +1294
flags.forEach(flag => {
if (inactiveFlags.has(flag)) {
throw new Error(`The flag '${flag}' is inactive: ${inactiveFlags.get(flag)}`);
}

if (!activeFlags.has(flag)) {
throw new Error(`Unknown flag '${flag}'.`);
}
});
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We also already have these checks in cli.js, where unknown flags result in exit code 2 but inactive flags only log a warning:

eslint/lib/cli.js

Lines 492 to 508 in 53b1ff0

if (eslintOptions.flags) {
debug("Checking for inactive flags");
for (const flag of eslintOptions.flags) {
if (inactiveFlags.has(flag)) {
log.warn(`InactiveFlag: The '${flag}' flag is no longer active: ${inactiveFlags.get(flag)}`);
continue;
}
if (activeFlags.has(flag)) {
continue;
}
log.error(`InvalidFlag: The '${flag}' flag is invalid.`);
return 2;
}
}

Now, for inactive flags passed on the command line, ESLint would log both a warning (from cli.js) and an error (from linter):

$ node bin/eslint Makefile.js --flag test_only_old
InactiveFlag: The 'test_only_old' flag is no longer active: Used only for testing.

Oops! Something went wrong! :(

ESLint: 9.7.0

Error: The flag 'test_only_old' is inactive: Used only for testing.
    at C:\projects\eslint\lib\linter\linter.js:1288:23
    at Array.forEach (<anonymous>)
    at new Linter (C:\projects\eslint\lib\linter\linter.js:1286:15)
    at new ESLint (C:\projects\eslint\lib\eslint\eslint.js:596:24)
    at Object.execute (C:\projects\eslint\lib\cli.js:510:24)
    at async main (C:\projects\eslint\bin\eslint.js:153:22)

Perhaps we should sync the behavior to avoid duplicate logs, or maybe just remove the checks in cli.js (if we don't expect to have any cli-level-only flags).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, these two tests are failing (at least for me locally, CI is broken at the moment) because now they result in throwing an error:

eslint/tests/lib/cli.js

Lines 1911 to 1923 in 53b1ff0

it("should emit a warning when an inactive flag is used", async () => {
const configPath = getFixturePath("eslint.config.js");
const filePath = getFixturePath("passing.js");
const input = `--flag test_only_old --config ${configPath} ${filePath}`;
const exitCode = await cli.execute(input, null, true);
sinon.assert.calledOnce(log.warn);
const formattedOutput = log.warn.firstCall.args[0];
assert.include(formattedOutput, `InactiveFlag: The 'test_only_old' flag is no longer active: ${inactiveFlags.get("test_only_old")}`);
assert.strictEqual(exitCode, 0);
});

it("should return false if the flag is present and inactive", () => {
eslint = new ESLint({ cwd: getFixturePath(), flags: ["test_only_old"] });
assert.strictEqual(eslint.hasFlag("test_only_old"), false);
});

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll update these.

Comment thread docs/src/use/formatters/index.md
Comment thread docs/src/pages/flags.md
@github-actions github-actions Bot added the cli Relates to ESLint's command-line interface label Jul 22, 2024
@mdjermanovic
Copy link
Copy Markdown
Member

This test still needs to be updated:

it("should return false if the flag is present and inactive", () => {
eslint = new ESLint({ cwd: getFixturePath(), flags: ["test_only_old"] });
assert.strictEqual(eslint.hasFlag("test_only_old"), false);
});

Comment thread tests/lib/cli.js Outdated
Comment thread tests/lib/cli.js Outdated
nzakas and others added 2 commits July 23, 2024 10:49
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Copy Markdown
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!

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 bug ESLint is working incorrectly cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants