fix: load plugins in the CLI in flat config mode #18185

fasttime · 2024-03-10T09:51:16Z

Prerequisites checklist

I have read the contributing guidelines.

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

[ ] 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:

Fixes #18159

What changes did you make? (Give an overview)

The changes in this PR relate to how the CLI option --plugin works in flat config mode:

When a plugin is loaded, the default property of the imported module is now used. This aligns the plugin loading behavior with eslintrc and with the recommendations for creating a custom plugin.
A specific error message is printed if a plugin has no default export.
Multiple plugins are loaded concurrently.

This PR also adds unit tests and a new note to the custom plugins docs.

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

Some of the new unit tests in this PR, the ones with CommonJS plugins, can be generalized to cover the eslintrc case. Do we want to add tests for eslintrc to make sure that the behavior in flat config mode is similar?

netlify · 2024-03-10T09:51:37Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`e6a5f4e`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/65f1417eb17cc900088caac9

JoshuaKGoldberg

LGTM! Just a few small questions/suggestions, nothing blocking IMO.

+1 on the tests, nice and thorough!

JoshuaKGoldberg · 2024-03-11T12:03:18Z

lib/cli.js

+
+    await Promise.all(pluginNames.map(async pluginName => {
+
+        const shortName = naming.getShorthandName(pluginName, "eslint-plugin");


[Style] Small thing: shortName isn't used until after the if. I think it'd make the code just a bit more clear to move the variable down a few lines. (Is this too nitpicky a comment?)

JoshuaKGoldberg · 2024-03-11T12:04:51Z

lib/cli.js

+        const module = await importer.import(longName);
+
+        if (!("default" in module)) {
+            throw Error(`"${longName}" cannot be imported because it does not provide a default export`);


[Question] Elsewhere outside of .md files and tests/, it's always throw new Error - not throw Error. I.e. no-throw-literal has been adhered to. Is it intentional that this doesn't?

Suggested change

throw Error(`"${longName}" cannot be imported because it does not provide a default export`);

throw new Error(`"${longName}" cannot be imported because it does not provide a default export`);

I think it's just a matter of style. throw Error() is the same as throw new Error(). no-throw-literal does not flag it (playground). In my repos, I have a rule that reports unnecessary new expressions, so I have a tendency to write just Error(). eslint-plugin-unicorn has this rule that requires new Error in throw statements.

docs/src/extend/plugins.md

aladdin-add · 2024-03-12T07:36:07Z

lib/cli.js

+/**
+ * Loads plugins with the specified names.
+ * @param {{ "import": (name: string) => Promise<any> }} importer An object with an `import` method called once for each plugin.
+ * @param {string[]} pluginNames The names of the plugins to be loaded, with or without the "eslint-plugin-" prefix.


It's fine to allow both, but we can start recommending using the full package name. In the long run, eslintrc will go away and plugin names are not enforced to be eslint-plugin-xxx.

@aladdin-add indeed the plugins used with this option must still follow eslintrc naming convention, but since this PR doesn't change that behavior, can you open a separate issue to discuss this?

mdjermanovic · 2024-03-12T09:00:51Z

Some of the new unit tests in this PR, the ones with CommonJS plugins, can be generalized to cover the eslintrc case. Do we want to add tests for eslintrc to make sure that the behavior in flat config mode is similar?

I think there's no need to add more tests for eslintrc since there are already some tests and the way the --plugin option is used and the plugins are loaded in eslintrc mode is entirely different.

aladdin-add

LGTM, thanks! 👍

mdjermanovic · 2024-03-12T09:12:33Z

tests/lib/cli.js

@@ -1649,6 +1649,105 @@ describe("cli", () => {
            });
        });

+        describe("flat Only", () => {


This was probably accidentally inserted in "eslintrc Only".

cli eslintrc Only flat Only `--plugin` option √ should load a plugin from a CommonJS package (41ms) √ should load a plugin from an ESM package √ should load multiple plugins √ should resolve plugins specified with 'eslint-plugin-' √ should resolve plugins in the parent directory's node_module subdirectory √ should fail if a plugin is not found √ should fail if a plugin throws an error while loading √ should fail to load a plugin from a package without a default export

Missed that, thanks! It's fixed in 34e02cb.

mdjermanovic · 2024-03-12T09:29:53Z

lib/cli.js

+        const module = await importer.import(longName);
+
+        if (!("default" in module)) {
+            throw new Error(`"${longName}" cannot be imported because it does not provide a default export`);


This sounds like the package doesn't provide a default export, but it does while the problem is that its default export doesn't have "default" name exported. Maybe something like this?

Suggested change

throw new Error(`"${longName}" cannot be imported because it does not provide a default export`);

throw new Error(`"${longName}" cannot be used with the `--plugin` option because its default module does not provide `default` export`);

Updated in e6a5f4e.

mdjermanovic

LGTM, thanks!

fix: load plugins in the CLI in flat config mode

24bfaac

eslint-github-bot bot added the bug ESLint is working incorrectly label Mar 10, 2024

github-actions bot added the cli Relates to ESLint's command-line interface label Mar 10, 2024

fasttime marked this pull request as ready for review March 11, 2024 10:58

fasttime requested a review from a team as a code owner March 11, 2024 10:58

JoshuaKGoldberg previously approved these changes Mar 11, 2024

View reviewed changes

JoshuaKGoldberg reviewed Mar 11, 2024

View reviewed changes

docs/src/extend/plugins.md Outdated Show resolved Hide resolved

mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Mar 11, 2024

apply review suggestions

b138d46

aladdin-add reviewed Mar 12, 2024

View reviewed changes

aladdin-add previously approved these changes Mar 12, 2024

View reviewed changes

mdjermanovic reviewed Mar 12, 2024

View reviewed changes

fasttime dismissed JoshuaKGoldberg’s stale review via b138d46 March 12, 2024 12:05

fasttime added 2 commits March 12, 2024 23:25

fix nesting of unit tests

34e02cb

improve error message

e6a5f4e

fasttime dismissed aladdin-add’s stale review via e6a5f4e March 13, 2024 06:02

mdjermanovic approved these changes Mar 13, 2024

View reviewed changes

mdjermanovic merged commit ae8103d into main Mar 13, 2024
19 checks passed

mdjermanovic deleted the issue-18159 branch March 13, 2024 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: load plugins in the CLI in flat config mode #18185

fix: load plugins in the CLI in flat config mode #18185

fasttime commented Mar 10, 2024

netlify bot commented Mar 10, 2024 •

edited

JoshuaKGoldberg left a comment •

edited

JoshuaKGoldberg Mar 11, 2024

JoshuaKGoldberg Mar 11, 2024

fasttime Mar 12, 2024

aladdin-add Mar 12, 2024 •

edited

mdjermanovic Mar 12, 2024

mdjermanovic commented Mar 12, 2024

aladdin-add left a comment

mdjermanovic Mar 12, 2024

fasttime Mar 13, 2024

mdjermanovic Mar 12, 2024

fasttime Mar 13, 2024

mdjermanovic left a comment


		await Promise.all(pluginNames.map(async pluginName => {

		const shortName = naming.getShorthandName(pluginName, "eslint-plugin");

	throw Error(`"${longName}" cannot be imported because it does not provide a default export`);
	throw new Error(`"${longName}" cannot be imported because it does not provide a default export`);

fix: load plugins in the CLI in flat config mode #18185

fix: load plugins in the CLI in flat config mode #18185

Conversation

fasttime commented Mar 10, 2024

Prerequisites checklist

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

What changes did you make? (Give an overview)

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

netlify bot commented Mar 10, 2024 • edited

✅ Deploy Preview for docs-eslint canceled.

JoshuaKGoldberg left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aladdin-add Mar 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjermanovic commented Mar 12, 2024

aladdin-add left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

netlify bot commented Mar 10, 2024 •

edited

JoshuaKGoldberg left a comment •

edited

aladdin-add Mar 12, 2024 •

edited