Fix: don't classify plugins that throw errors as "missing" (fixes #6874)

[not-an-aardvark]

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

[x] Bug fix (#6874)

What changes did you make? (Give an overview)

Previously, if a plugin threw an error on load, ESLint would display the "missing plugin" error message. This was confusing for plugin developers, because it would be difficult to distinguish between an installation error and a broken plugin.

This fix avoids the confusion by treating the errors as fatal and crashing with a stack trace if a plugin throws when it loads, as described in #7668 (comment).

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

Nothing in particular

[mention-bot]

@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @mysticatea and @alberto to be potential reviewers.

[eslintbot]

LGTM

[not-an-aardvark]

I couldn't think of a good way to test the "throwing on load" behavior (proxyquire doesn't seem to support this case), so I just published a real plugin that throws on load for the purposes of this test.

[ilyavolodin]

Proxyquire supports mocking a dependency with any function. If you throw inside that function, it should work.

[not-an-aardvark]

I tried doing this:

diff --git a/tests/lib/config/plugins.js b/tests/lib/config/plugins.js
index 8bbff180..433cbeb8 100644
--- a/tests/lib/config/plugins.js
+++ b/tests/lib/config/plugins.js
@@ -43,7 +43,10 @@ describe("Plugins", () => {
                 "eslint-plugin-example": plugin,
                 "@scope/eslint-plugin-example": scopedPlugin,
                 "./environments": Environments,
-                "../rules": Rules
+                "../rules": Rules,
+                "eslint-plugin-throws-on-load"() {
+                    throw new Error("error thrown while loading this module");
+                }
             });
         });
 

But it doesn't work -- it seems like the function that throws an error is getting returned as the module rather than getting called. Is there another way to do it?

[vitorbal]

I think if you mock a dependency with a function that just means that the module exports a function.. I tried checking the docs of proxyquire and couldn't find any way to do some "initialization" logic before the stubbed exports object.

How about something with a getter, like this:

diff --git a/tests/lib/config/plugins.js b/tests/lib/config/plugins.js
index 8bbff18..f687343 100644
--- a/tests/lib/config/plugins.js
+++ b/tests/lib/config/plugins.js
@@ -42,6 +42,11 @@ describe("Plugins", () => {
             StubbedPlugins = proxyquire("../../../lib/config/plugins", {
                 "eslint-plugin-example": plugin,
                 "@scope/eslint-plugin-example": scopedPlugin,
+                "eslint-plugin-throws-on-load": {
+                    get rules() {
+                        throw new Error('error thrown while loading this module');
+                    }
+                },
                 "./environments": Environments,
                 "../rules": Rules
             });

I ran the tests locally and that seems to work, but it relies on the fact that the rules get registered when the plugin is loaded via Plugins.load.

[platinumazure]

Basically LGTM, but I'm hoping our best minds can find a different approach for testing.

[eslintbot]

LGTM