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

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

Merged
merged 4 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/src/extend/plugins.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ export default plugin;
module.exports = plugin;
```

If you plan to distribute your plugin as an npm package, make sure that the module that exports the plugin object is the default export of your package. This will enable ESLint to import the plugin when it is specified in the command line in the [`--plugin` option](../use/command-line-interface#--plugin).

### Meta Data in Plugins

For easier debugging and more effective caching of plugins, it's recommended to provide a name and version in a `meta` object at the root of your plugin, like this:
Expand Down
39 changes: 28 additions & 11 deletions lib/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const debug = require("debug")("eslint:cli");
/** @typedef {import("./eslint/eslint").LintMessage} LintMessage */
/** @typedef {import("./eslint/eslint").LintResult} LintResult */
/** @typedef {import("./options").ParsedCLIOptions} ParsedCLIOptions */
/** @typedef {import("./shared/types").Plugin} Plugin */
/** @typedef {import("./shared/types").ResultsMeta} ResultsMeta */

//------------------------------------------------------------------------------
Expand All @@ -47,6 +48,32 @@ const mkdir = promisify(fs.mkdir);
const stat = promisify(fs.stat);
const writeFile = promisify(fs.writeFile);

/**
* 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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

* @returns {Promise<Record<string, Plugin>>} A mapping of plugin short names to implementations.
*/
async function loadPlugins(importer, pluginNames) {
const plugins = {};

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

const longName = naming.normalizePackageName(pluginName, "eslint-plugin");
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`);
Copy link
Member

Choose a reason for hiding this comment

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

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`);

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in e6a5f4e.

}

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

plugins[shortName] = module.default;
}));

return plugins;
}

/**
* Predicate function for whether or not to apply fixes in quiet mode.
* If a message is a warning, do not apply a fix.
Expand Down Expand Up @@ -152,17 +179,7 @@ async function translateOptions({
}

if (plugin) {
const plugins = {};

for (const pluginName of plugin) {

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

plugins[shortName] = await importer.import(longName);
}

overrideConfig[0].plugins = plugins;
overrideConfig[0].plugins = await loadPlugins(importer, plugin);
}

} else {
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Empty file.
99 changes: 99 additions & 0 deletions tests/lib/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -1649,6 +1649,105 @@ describe("cli", () => {
});
});

describe("flat Only", () => {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


describe("`--plugin` option", () => {

let originalCwd;

beforeEach(() => {
originalCwd = process.cwd();
process.chdir(getFixturePath("plugins"));
});

afterEach(() => {
process.chdir(originalCwd);
originalCwd = void 0;
});

it("should load a plugin from a CommonJS package", async () => {
const code = "--plugin hello-cjs --rule 'hello-cjs/hello: error' ../files/*.js";

const exitCode = await cli.execute(code, null, true);

assert.strictEqual(exitCode, 1);
assert.ok(log.info.calledOnce);
assert.include(log.info.firstCall.firstArg, "Hello CommonJS!");
});

it("should load a plugin from an ESM package", async () => {
const code = "--plugin hello-esm --rule 'hello-esm/hello: error' ../files/*.js";

const exitCode = await cli.execute(code, null, true);

assert.strictEqual(exitCode, 1);
assert.ok(log.info.calledOnce);
assert.include(log.info.firstCall.firstArg, "Hello ESM!");
});

it("should load multiple plugins", async () => {
const code = "--plugin 'hello-cjs, hello-esm' --rule 'hello-cjs/hello: warn, hello-esm/hello: error' ../files/*.js";

const exitCode = await cli.execute(code, null, true);

assert.strictEqual(exitCode, 1);
assert.ok(log.info.calledOnce);
assert.include(log.info.firstCall.firstArg, "Hello CommonJS!");
assert.include(log.info.firstCall.firstArg, "Hello ESM!");
});

it("should resolve plugins specified with 'eslint-plugin-'", async () => {
const code = "--plugin 'eslint-plugin-schema-array, @scope/eslint-plugin-example' --rule 'schema-array/rule1: warn, @scope/example/test: warn' ../passing.js";

const exitCode = await cli.execute(code, null, true);

assert.strictEqual(exitCode, 0);
});

it("should resolve plugins in the parent directory's node_module subdirectory", async () => {
process.chdir("subdir");
const code = "--plugin 'example, @scope/example' file.js";

const exitCode = await cli.execute(code, null, true);

assert.strictEqual(exitCode, 0);
});

it("should fail if a plugin is not found", async () => {
const code = "--plugin 'example, no-such-plugin' ../passing.js";

await stdAssert.rejects(
cli.execute(code, null, true),
({ message }) => {
assert(
message.startsWith("Cannot find module 'eslint-plugin-no-such-plugin'\n"),
`Unexpected error message:\n${message}`
);
return true;
}
);
});

it("should fail if a plugin throws an error while loading", async () => {
const code = "--plugin 'example, throws-on-load' ../passing.js";

await stdAssert.rejects(
cli.execute(code, null, true),
{ message: "error thrown while loading this module" }
);
});

it("should fail to load a plugin from a package without a default export", async () => {
const code = "--plugin 'example, no-default-export' ../passing.js";

await stdAssert.rejects(
cli.execute(code, null, true),
{ message: '"eslint-plugin-no-default-export" cannot be imported because it does not provide a default export' }
);
});
});
});

describe("when loading a custom rule", () => {
it("should return an error when rule isn't found", async () => {
const rulesPath = getFixturePath("rules", "wrong");
Expand Down