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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Corrected notice for invalid (:) plugin names #13473

Merged
merged 7 commits into from Sep 12, 2020
Merged
Show file tree
Hide file tree
Changes from 5 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
16 changes: 11 additions & 5 deletions lib/cli-engine/config-array-factory.js
Expand Up @@ -286,14 +286,15 @@ function loadESLintIgnoreFile(filePath) {
* Creates an error to notify about a missing config to extend from.
* @param {string} configName The name of the missing config.
* @param {string} importerName The name of the config that imported the missing config
* @param {string} messageTemplate The text template to source error strings from.
* @returns {Error} The error object to throw
* @private
*/
function configMissingError(configName, importerName) {
function configInvalidError(configName, importerName, messageTemplate) {
return Object.assign(
new Error(`Failed to load config "${configName}" to extend from.`),
{
messageTemplate: "extend-config-missing",
messageTemplate,
messageData: { configName, importerName }
}
);
Expand Down Expand Up @@ -802,7 +803,7 @@ class ConfigArrayFactory {
});
}

throw configMissingError(extendName, ctx.name);
throw configInvalidError(extendName, ctx.name, "extend-config-missing");
}

/**
Expand All @@ -814,6 +815,11 @@ class ConfigArrayFactory {
*/
_loadExtendedPluginConfig(extendName, ctx) {
const slashIndex = extendName.lastIndexOf("/");

if (slashIndex === -1) {
throw configInvalidError(extendName, ctx.filePath, "plugin-invalid");
}

const pluginName = extendName.slice("plugin:".length, slashIndex);
const configName = extendName.slice(slashIndex + 1);

Expand All @@ -834,7 +840,7 @@ class ConfigArrayFactory {
});
}

throw plugin.error || configMissingError(extendName, ctx.filePath);
throw plugin.error || configInvalidError(extendName, ctx.filePath, "extend-config-missing");
}

/**
Expand Down Expand Up @@ -867,7 +873,7 @@ class ConfigArrayFactory {
} catch (error) {
/* istanbul ignore else */
if (error && error.code === "MODULE_NOT_FOUND") {
throw configMissingError(extendName, ctx.filePath);
throw configInvalidError(extendName, ctx.filePath, "extend-config-missing");
}
throw error;
}
Expand Down
8 changes: 8 additions & 0 deletions messages/plugin-invalid.txt
@@ -0,0 +1,8 @@
"<%= configName %>" is invalid syntax for a config specifier.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

* If your intention is to extend from a configuration exported from the plugin, add the configuration name after a slash: e.g. "<%= configName %>/myConfig".
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
* If this is the name of a shareable config instead of a plugin, remove the "plugin:" prefix: i.e. "<%= configName.slice("plugin:".length) %>".
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL this is allowed in _.template. Confirmed locally this does what we want it to:

Oops! Something went wrong! :(

ESLint: 7.7.0

"plugin:wat" is invalid syntax for a config specifier.

* If your intention is to extend from a configuration exported from the plugin, add the configuration name after a slash: e.g. "plugin:wat/myConfig".
* If this is the name of a shareable config instead of a plugin, remove the "plugin:" prefix: i.e. "wat".

'"plugin:wat"' was referenced from the config file in "C:\Code\eslinttemp\.eslintrc.json".

If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.

Copy link
Member

Choose a reason for hiding this comment

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

This output looks awesome!

I can also confirm locally that this works. It seems that <%- and <%= can evaluate code, although it doesn't look like that in the lodash docs.

JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

'"<%- configName %>"' was referenced from the config file in "<%- importerName %>".
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
17 changes: 17 additions & 0 deletions tests/lib/cli-engine/config-array-factory.js
Expand Up @@ -1515,6 +1515,23 @@ describe("ConfigArrayFactory", () => {
}, /Failed to load config "plugin:invalid-config\/bar" to extend from./u);
});

it("should throw an error with a message template when a plugin referenced for a plugin config is invalid", () => {
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
try {
applyExtends({
extends: "plugin:nonexistent-plugin",
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
rules: { eqeqeq: 2 }
});
} catch (err) {
assert.strictEqual(err.messageTemplate, "plugin-invalid");
assert.deepStrictEqual(err.messageData, {
configName: "plugin:nonexistent-plugin",
importerName: path.join(process.cwd(), "whatever")
});
return;
}
assert.fail("Expected to throw an error");
});

it("should throw an error with a message template when a plugin referenced for a plugin config is not found", () => {
try {
applyExtends({
Expand Down