From 35a3bd2d2e000f4dc3339261c43207a8e9e0b58f Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Tue, 7 Jul 2020 17:07:14 -0400 Subject: [PATCH 1/6] Fix: Added more specific notice for invalid (:) plugin names --- lib/cli-engine/config-array-factory.js | 4 ++-- lib/shared/naming.js | 2 +- messages/plugin-invalid.txt | 9 +++++++++ tests/lib/cli-engine/config-array-factory.js | 18 ++++++++++++++++++ tests/lib/shared/naming.js | 3 ++- 5 files changed, 32 insertions(+), 4 deletions(-) create mode 100644 messages/plugin-invalid.txt diff --git a/lib/cli-engine/config-array-factory.js b/lib/cli-engine/config-array-factory.js index 7c0fba65c67..06357d0197d 100644 --- a/lib/cli-engine/config-array-factory.js +++ b/lib/cli-engine/config-array-factory.js @@ -802,7 +802,7 @@ class ConfigArrayFactory { */ _loadExtendedPluginConfig(extendName, ctx) { const slashIndex = extendName.lastIndexOf("/"); - const pluginName = extendName.slice("plugin:".length, slashIndex); + const pluginName = slashIndex === -1 ? extendName : extendName.slice("plugin:".length, slashIndex); const configName = extendName.slice(slashIndex + 1); if (isFilePath(pluginName)) { @@ -994,7 +994,7 @@ class ConfigArrayFactory { error = resolveError; /* istanbul ignore else */ if (error && error.code === "MODULE_NOT_FOUND") { - error.messageTemplate = "plugin-missing"; + error.messageTemplate = request.indexOf(":") === -1 ? "plugin-missing" : "plugin-invalid"; error.messageData = { pluginName: request, resolvePluginsRelativeTo: ctx.pluginBasePath, diff --git a/lib/shared/naming.js b/lib/shared/naming.js index 32cff94538a..e959a18c035 100644 --- a/lib/shared/naming.js +++ b/lib/shared/naming.js @@ -43,7 +43,7 @@ function normalizePackageName(name, prefix) { */ normalizedName = normalizedName.replace(/^@([^/]+)\/(.*)$/u, `@$1/${prefix}-$2`); } - } else if (!normalizedName.startsWith(`${prefix}-`)) { + } else if (!normalizedName.startsWith(`${prefix}-`) && normalizedName.indexOf(":") === -1) { normalizedName = `${prefix}-${normalizedName}`; } diff --git a/messages/plugin-invalid.txt b/messages/plugin-invalid.txt new file mode 100644 index 00000000000..6d6e39d80b0 --- /dev/null +++ b/messages/plugin-invalid.txt @@ -0,0 +1,9 @@ +ESLint couldn't find the plugin "<%- pluginName %>". + +(The package "<%- pluginName %>" was not found when loaded as a Node module from the directory "<%- resolvePluginsRelativeTo %>".) + +It looks like this might not be a valid plugin name. Did you use the name of a preset configuration instead of a plugin? + +The plugin "<%- pluginName %>" was referenced from the config file in "<%- importerName %>". + +If you still can't figure out the problem, please stop by https://eslint.org/chat to chat with the team. diff --git a/tests/lib/cli-engine/config-array-factory.js b/tests/lib/cli-engine/config-array-factory.js index 3b979c1d38f..90c80e3672f 100644 --- a/tests/lib/cli-engine/config-array-factory.js +++ b/tests/lib/cli-engine/config-array-factory.js @@ -1503,6 +1503,24 @@ 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", () => { + try { + applyExtends({ + extends: "plugin:nonexistent-plugin", + rules: { eqeqeq: 2 } + }); + } catch (err) { + assert.strictEqual(err.messageTemplate, "plugin-invalid"); + assert.deepStrictEqual(err.messageData, { + pluginName: "plugin:nonexistent-plugin", + resolvePluginsRelativeTo: process.cwd(), + importerName: "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({ diff --git a/tests/lib/shared/naming.js b/tests/lib/shared/naming.js index 0a868a872ed..8e6fdb99d85 100644 --- a/tests/lib/shared/naming.js +++ b/tests/lib/shared/naming.js @@ -25,7 +25,8 @@ describe("naming", () => { ["@z\\foo", "@z/eslint-config-foo"], ["@z\\foo\\bar.js", "@z/eslint-config-foo/bar.js"], ["@z/eslint-config", "@z/eslint-config"], - ["@z/eslint-config-foo", "@z/eslint-config-foo"] + ["@z/eslint-config-foo", "@z/eslint-config-foo"], + ["plugin:invalid", "plugin:invalid"] ], (input, expected) => { it(`should return ${expected} when passed ${input}`, () => { const result = naming.normalizePackageName(input, "eslint-config"); From b3baa257f7697acc59abdd621ef498d7efadbd83 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Wed, 26 Aug 2020 21:59:43 -0400 Subject: [PATCH 2/6] PR feedback: better .txt; throwing immediately --- lib/cli-engine/config-array-factory.js | 20 +++++++++++++------- messages/plugin-invalid.txt | 4 ++-- tests/lib/cli-engine/config-array-factory.js | 5 ++--- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/lib/cli-engine/config-array-factory.js b/lib/cli-engine/config-array-factory.js index 06357d0197d..eafe68f1232 100644 --- a/lib/cli-engine/config-array-factory.js +++ b/lib/cli-engine/config-array-factory.js @@ -274,14 +274,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 tempalte 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 } } ); @@ -790,7 +791,7 @@ class ConfigArrayFactory { }); } - throw configMissingError(extendName, ctx.name); + throw configInvalidError(extendName, ctx.name, "extend-config-missing"); } /** @@ -802,7 +803,12 @@ class ConfigArrayFactory { */ _loadExtendedPluginConfig(extendName, ctx) { const slashIndex = extendName.lastIndexOf("/"); - const pluginName = slashIndex === -1 ? extendName : extendName.slice("plugin:".length, slashIndex); + + if (slashIndex === -1) { + throw configInvalidError(extendName, ctx.filePath, "plugin-invalid"); + } + + const pluginName = extendName.slice("plugin:".length, slashIndex); const configName = extendName.slice(slashIndex + 1); if (isFilePath(pluginName)) { @@ -822,7 +828,7 @@ class ConfigArrayFactory { }); } - throw plugin.error || configMissingError(extendName, ctx.filePath); + throw plugin.error || configInvalidError(extendName, ctx.filePath, "extend-config-missing"); } /** @@ -855,7 +861,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; } @@ -994,7 +1000,7 @@ class ConfigArrayFactory { error = resolveError; /* istanbul ignore else */ if (error && error.code === "MODULE_NOT_FOUND") { - error.messageTemplate = request.indexOf(":") === -1 ? "plugin-missing" : "plugin-invalid"; + error.messageTemplate = "plugin-missing"; error.messageData = { pluginName: request, resolvePluginsRelativeTo: ctx.pluginBasePath, diff --git a/messages/plugin-invalid.txt b/messages/plugin-invalid.txt index 6d6e39d80b0..ead6da6b5dc 100644 --- a/messages/plugin-invalid.txt +++ b/messages/plugin-invalid.txt @@ -2,8 +2,8 @@ ESLint couldn't find the plugin "<%- pluginName %>". (The package "<%- pluginName %>" was not found when loaded as a Node module from the directory "<%- resolvePluginsRelativeTo %>".) -It looks like this might not be a valid plugin name. Did you use the name of a preset configuration instead of a plugin? +It looks like this might not be a valid plugin name. Did you use the name of a shareable config instead of a plugin? The plugin "<%- pluginName %>" was referenced from the config file in "<%- importerName %>". -If you still can't figure out the problem, please stop by https://eslint.org/chat to chat with the team. +If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. diff --git a/tests/lib/cli-engine/config-array-factory.js b/tests/lib/cli-engine/config-array-factory.js index 90c80e3672f..64b477e539d 100644 --- a/tests/lib/cli-engine/config-array-factory.js +++ b/tests/lib/cli-engine/config-array-factory.js @@ -1512,9 +1512,8 @@ describe("ConfigArrayFactory", () => { } catch (err) { assert.strictEqual(err.messageTemplate, "plugin-invalid"); assert.deepStrictEqual(err.messageData, { - pluginName: "plugin:nonexistent-plugin", - resolvePluginsRelativeTo: process.cwd(), - importerName: "whatever" + configName: "plugin:nonexistent-plugin", + importerName: path.join(process.cwd(), "whatever") }); return; } From abdb018de9bdcaf8ef934a0c1f109b2788f7ebf6 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Wed, 26 Aug 2020 22:12:04 -0400 Subject: [PATCH 3/6] Typo in lib/cli-engine/config-array-factory.js --- lib/cli-engine/config-array-factory.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cli-engine/config-array-factory.js b/lib/cli-engine/config-array-factory.js index 356600d6219..2c7a79b491e 100644 --- a/lib/cli-engine/config-array-factory.js +++ b/lib/cli-engine/config-array-factory.js @@ -286,7 +286,7 @@ 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 tempalte to source error strings from. + * @param {string} messageTemplate The text template to source error strings from. * @returns {Error} The error object to throw * @private */ From 374dac517a3c2ef93b68f59ae36f14da3382d9d7 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 29 Aug 2020 16:35:46 -0400 Subject: [PATCH 4/6] Improve plugin-invalid.txt --- messages/plugin-invalid.txt | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/messages/plugin-invalid.txt b/messages/plugin-invalid.txt index ead6da6b5dc..012cbfdc730 100644 --- a/messages/plugin-invalid.txt +++ b/messages/plugin-invalid.txt @@ -1,9 +1,8 @@ -ESLint couldn't find the plugin "<%- pluginName %>". +"<%= configName %>" is invalid syntax for a config specifier. -(The package "<%- pluginName %>" was not found when loaded as a Node module from the directory "<%- resolvePluginsRelativeTo %>".) +* If your intention is to extend from a configuration exported from the plugin, add the configuration name after a slash: e.g. "<%= configName %>/myConfig". +* If this is the name of a shareable config instead of a plugin, remove the "plugin:" prefix: i.e. "<%= configName.slice("plugin:".length) %>". -It looks like this might not be a valid plugin name. Did you use the name of a shareable config instead of a plugin? - -The plugin "<%- pluginName %>" was referenced from the config file in "<%- importerName %>". +'"<%- configName %>"' was referenced from the config file in "<%- importerName %>". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. From fb9af040ed0f5805d603152497ce02e8d49c44e1 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sun, 6 Sep 2020 10:54:07 -0400 Subject: [PATCH 5/6] Apply suggestions from code review Co-authored-by: Milos Djermanovic --- messages/plugin-invalid.txt | 8 ++++---- tests/lib/cli-engine/config-array-factory.js | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/messages/plugin-invalid.txt b/messages/plugin-invalid.txt index 012cbfdc730..3ee251821be 100644 --- a/messages/plugin-invalid.txt +++ b/messages/plugin-invalid.txt @@ -1,8 +1,8 @@ -"<%= configName %>" is invalid syntax for a config specifier. +"<%- configName %>" 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. "<%= configName %>/myConfig". -* If this is the name of a shareable config instead of a plugin, remove the "plugin:" prefix: i.e. "<%= configName.slice("plugin:".length) %>". +* If your intention is to extend from a configuration exported from the plugin, add the configuration name after a slash: e.g. "<%- configName %>/myConfig". +* If this is the name of a shareable config instead of a plugin, remove the "plugin:" prefix: i.e. "<%- configName.slice("plugin:".length) %>". -'"<%- configName %>"' was referenced from the config file in "<%- importerName %>". +"<%- configName %>" was referenced from the config file in "<%- importerName %>". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. diff --git a/tests/lib/cli-engine/config-array-factory.js b/tests/lib/cli-engine/config-array-factory.js index 266f6615db2..27b84c68de7 100644 --- a/tests/lib/cli-engine/config-array-factory.js +++ b/tests/lib/cli-engine/config-array-factory.js @@ -1515,7 +1515,7 @@ 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", () => { + it("should throw an error with a message template when a plugin config specifier is missing config name", () => { try { applyExtends({ extends: "plugin:nonexistent-plugin", From a05ef0515501d89ce6e2acd8d8a0259d6069ae60 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sun, 6 Sep 2020 10:58:52 -0400 Subject: [PATCH 6/6] Test name improvement --- tests/lib/cli-engine/config-array-factory.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/lib/cli-engine/config-array-factory.js b/tests/lib/cli-engine/config-array-factory.js index 27b84c68de7..6df3d98372b 100644 --- a/tests/lib/cli-engine/config-array-factory.js +++ b/tests/lib/cli-engine/config-array-factory.js @@ -1518,13 +1518,13 @@ describe("ConfigArrayFactory", () => { it("should throw an error with a message template when a plugin config specifier is missing config name", () => { try { applyExtends({ - extends: "plugin:nonexistent-plugin", + extends: "plugin:some-plugin", rules: { eqeqeq: 2 } }); } catch (err) { assert.strictEqual(err.messageTemplate, "plugin-invalid"); assert.deepStrictEqual(err.messageData, { - configName: "plugin:nonexistent-plugin", + configName: "plugin:some-plugin", importerName: path.join(process.cwd(), "whatever") }); return;