core: support local plugins from global Lighthouse #11696

koh110 · 2020-11-22T16:02:37Z

Summary

Lighthouse CLI will be able to loading plugins in node_modules/.

bugfix

Lighthouse CLI installed in global failed loading plugins in current working directory.

$ lighthouse https://www.google.com --config-path=./config.js
Runtime error encountered: Unable to locate plugin: `lighthouse-plugin-field-performance`.
     Tried to require() from these locations:
     ...
Error: Unable to locate plugin: `lighthouse-plugin-field-performance`.
     Tried to require() from these locations:

$ which lighthouse
/usr/local/bin/lighthouse
$ cat package.json
{
  "private": true,
  "dependencies": {
    "lighthouse-plugin-field-performance": "^2.2.1"
  }
}

Related Issues/PRs

Ref: #11671

...se-core/test/fixtures/config-helpers/lighthouse-plugin-config-helper/plugin-config-helper.js

lighthouse-core/test/config/config-helpers-test.js

lighthouse-core/config/config-helpers.js

adamraine · 2020-11-23T17:06:26Z

lighthouse-core/config/config-helpers.js

+  // |-------------------------------------------|---------------------------------------|
+  // | absolute paths                            |   1.                                  |
+  // | relative to the current working directory |   3.                                  |
+  // | relative to the config.js file            |   4.                                  |


Thanks for these table breakdowns!

lighthouse-core/test/config/config-helpers-test.js

connorjclark · 2020-11-24T16:04:41Z

.gitignore

+!lighthouse-core/test/fixtures/config-helpers/config/node_modules
+!lighthouse-core/test/fixtures/config-helpers/node_modules


Suggested change

!lighthouse-core/test/fixtures/config-helpers/config/node_modules

!lighthouse-core/test/fixtures/config-helpers/node_modules

!lighthouse-core/test/fixtures/config-helpers/**/*/node_modules

Thanks for your suggestion!
When I tried, the rule seems to ignore lighthouse-core/test/fixtures/config-helpers/node_modules.

-!lighthouse-core/test/fixtures/config-helpers/config/node_modules -!lighthouse-core/test/fixtures/config-helpers/node_modules +!lighthouse-core/test/fixtures/config/**/*/node_modules

deleted: lighthouse-core/test/fixtures/config-helpers/node_modules/plugin-in-working-directory/package.json deleted: lighthouse-core/test/fixtures/config-helpers/node_modules/plugin-in-working-directory/plugin-in-working-directory.js renamed: lighthouse-core/test/fixtures/config-helpers/config/config.js -> lighthouse-core/test/fixtures/config/config/config.js renamed: lighthouse-core/test/fixtures/config-helpers/config/node_modules/plugin-in-config-directory/package.json -> lighthouse-core/test/fixtures/config/config/node_modules/plugin-in-config-directory/package.json renamed: lighthouse-core/test/fixtures/config-helpers/config/node_modules/plugin-in-config-directory/plugin-in-config-directory.js -> lighthouse-core/test/fixtures/config/config/node_modules/plugin-in-config-directory/plugin-in-config-directory.js renamed: lighthouse-core/test/fixtures/config-helpers/lighthouse-plugin-config-helper/package.json -> lighthouse-core/test/fixtures/config/lighthouse-plugin-config-helper/package.json renamed: lighthouse-core/test/fixtures/config-helpers/lighthouse-plugin-config-helper/plugin-config-helper.js -> lighthouse-core/test/fixtures/config/lighthouse-plugin-config-helper/plugin-config-helper.js

Am I doing something wrong?

oh, oops, should just be !lighthouse-core/test/fixtures/config/**/node_modules

Aah! It worked perfectly! Thanks a lot!

connorjclark · 2020-11-24T16:05:49Z

lighthouse-core/test/fixtures/config-helpers/lighthouse-plugin-config-helper/package.json

@@ -0,0 +1,5 @@
+{


Can you rename this folder to just config (instead of config-helpers)?

OK. I'll do it!

lighthouse-core/test/config/config-helpers-test.js

...thouse-core/test/fixtures/config/config/node_modules/plugin-in-config-directory/package.json

connorjclark · 2020-11-24T17:40:45Z

lighthouse-core/test/config/config-helpers-test.js

+    //     |-- package.json
+    it('relative to the config path', () => {
+      const pluginName = 'plugin-in-config-directory';
+      const pluginDir = path.join(pluginsDirectory, 'config', 'node_modules', 'plugin-in-config-directory');


You can just do

`${pluginsDirectory}/config/node_modules/plugin-in-config-directory`

connorjclark · 2020-11-25T20:30:38Z

lighthouse-core/test/config/config-helpers-test.js

+    const configFixturePath = path.resolve(__dirname, '../fixtures/config');
+
+    beforeEach(() => {
+      process.cwd = jest.fn(() => configFixturePath);


Does process.cwd need to be restored after?

I don't think so. The idea was to provide a default cwd so tests which set cwd don't affect other tests.

@koh110 I think this beforeEach should apply to every test not just this block.

ah I missed L14, that will restore process.

@adamraine I agree with your comment. I will move up beforeEach.

adamraine

LGTM

brendankenny

some suggestions to maybe make the comments a little clearer, feel free to take or leave any of them :)

brendankenny · 2020-11-30T17:31:45Z

...thouse-core/test/fixtures/config/config/node_modules/plugin-in-config-directory/package.json

@@ -0,0 +1,5 @@
+{


did these files accidentally get double nested in a config directory? lighthouse-core/test/fixtures/config/config/...

@brendankenny
No. it is not accident.
It was named lighthouse-core/test/fixtures/config-helpers/config at first.
I was suggested renaming this folder.
#11696 (comment)

Do you think I should rename to another name?

brendankenny · 2020-11-30T17:57:54Z

lighthouse-core/config/config-helpers.js

@@ -176,6 +176,21 @@ function requireAudits(audits, configDir) {
 * @throws {Error}
 */
 function resolveModule(moduleIdentifier, configDir, category) {
+  // node_modules/


Suggested change

// node_modules/

// module in a node_modules/ that is...

brendankenny · 2020-11-30T17:59:37Z

lighthouse-core/config/config-helpers.js

+  // node_modules/
+  // |                                | Lighthouse globally installed | Lighthouse locally installed |
+  // |--------------------------------|-------------------------------|------------------------------|
+  // | in global                      |   1.                          |   1.                         |


Suggested change

// | in global | 1. | 1. |

// | global | 1. | 1. |

brendankenny · 2020-11-30T18:00:00Z

lighthouse-core/config/config-helpers.js

+  // |                                | Lighthouse globally installed | Lighthouse locally installed |
+  // |--------------------------------|-------------------------------|------------------------------|
+  // | in global                      |   1.                          |   1.                         |
+  // | current working directory      |   2.                          |   1.                         |


Suggested change

// | current working directory | 2. | 1. |

// | in current working directory | 2. | 1. |

brendankenny · 2020-11-30T18:01:02Z

lighthouse-core/config/config-helpers.js

+  // | current working directory      |   2.                          |   1.                         |
+  // | relative to config.js file     |   5.                          |   -                          |
+
+  // path to file


Suggested change

// path to file

// module given by a path that is...

brendankenny · 2020-11-30T18:01:23Z

lighthouse-core/config/config-helpers.js

+  // path to file
+  // |                                           | Lighthouse globally/locally installed |
+  // |-------------------------------------------|---------------------------------------|
+  // | absolute paths                            |   1.                                  |


Suggested change

// | absolute paths | 1. |

// | absolute | 1. |

brendankenny · 2020-11-30T18:01:42Z

lighthouse-core/config/config-helpers.js

@@ -184,6 +199,18 @@ function resolveModule(moduleIdentifier, configDir, category) {
    return require.resolve(moduleIdentifier);
  } catch (e) {}

+  // 2.
+  // Lighthouse globally installed, node_modules/ in current working directoriy.


Suggested change

// Lighthouse globally installed, node_modules/ in current working directoriy.

// Lighthouse globally installed, node_modules/ in current working directory.

brendankenny · 2020-11-30T18:07:18Z

lighthouse-core/config/config-helpers.js

  // relative path passed to `require()` is found relative to the file it's
  // in, this allows module paths to be specified relative to the config file.
  const relativePath = path.resolve(configDir, moduleIdentifier);
  try {
    return require.resolve(relativePath);
  } catch (requireError) {}

+  // 5.
+  // Lighthouse globally installed, node_modules/ in config directoriy.


Suggested change

// Lighthouse globally installed, node_modules/ in config directoriy.

// Lighthouse globally installed, node_modules/ in config directory.

lighthouse-core/config/config-helpers.js

brendankenny · 2020-11-30T19:55:43Z

lighthouse-core/test/config/config-helpers-test.js

+
+    it('relative to the config path', () => {
+      const pluginName = 'lighthouse-plugin-config-helper';
+      const pathToPlugin = resolveModule(pluginName, configFixturePath, 'plugin');


this one isn't testing the relative-to-the-configDir case because the default process.cwd set in beforeEach is also that same path. Need to set process.cwd to something else to test it (I assume that's what the '/usr/bin/node' below is doing for the other "config path" test)

Thanks. I move up process.cwd to upper directory.

koh110 · 2020-12-10T11:26:35Z

@brendankenny I've modified the code, can you check it for me?

brendankenny

sorry for the delay! I'll land the suggestion and then LGTM!

lighthouse-core/config/config-helpers.js

brendankenny · 2020-12-11T21:30:32Z

Thanks @koh110! A great fix for what was a big hole in the plugin system.

koh110 · 2020-12-12T15:56:57Z

Oops, I forget to fix. I appreciate it! Thanks @brendankenny
#11696 (comment)

paulirish · 2020-12-14T06:48:44Z

Incredible work, @koh110! We really appreciate it. :D

koh110 · 2020-12-14T15:40:27Z

@paulirish Lighthouse has helped me with both my work and my hobby of code.
So I'm happy if my contribution can help you guys!

core(config-helper): load plugins in node_modules

a95661d

koh110 requested a review from a team as a code owner November 22, 2020 16:02

koh110 requested review from adamraine and removed request for a team November 22, 2020 16:02

google-cla bot added the cla: yes label Nov 22, 2020

vercel bot deployed to Preview November 22, 2020 16:02 View deployment

devtools-bot assigned adamraine Nov 22, 2020

devtools-bot added the waiting4reviewer label Nov 22, 2020

connorjclark changed the title ~~core(config-helper): load plugins in node_modules/~~ core(config): load plugins in node_modules/ Nov 23, 2020

adamraine reviewed Nov 23, 2020

View reviewed changes

connorjclark changed the title ~~core(config): load plugins in node_modules/~~ core(config): load plugins from node_modules/ Nov 23, 2020

adamraine added waiting4committer and removed waiting4reviewer labels Nov 23, 2020

fix from code review

ebf7e3c

vercel bot deployed to Preview November 24, 2020 12:32 View deployment

connorjclark reviewed Nov 24, 2020

View reviewed changes

Apply suggestions from code review

055372f

vercel bot deployed to Preview November 24, 2020 16:30 View deployment

adamraine reviewed Nov 24, 2020

View reviewed changes

connorjclark reviewed Nov 24, 2020

View reviewed changes

Apply suggestions from code review

4b48002

vercel bot deployed to Preview November 25, 2020 16:05 View deployment

connorjclark reviewed Nov 25, 2020

View reviewed changes

vercel bot deployed to Preview November 28, 2020 08:13 View deployment

apply comments

267caed

koh110 force-pushed the plugin branch from cd929e6 to 267caed Compare November 28, 2020 09:52

vercel bot deployed to Preview November 28, 2020 09:52 View deployment

koh110 requested review from connorjclark and adamraine November 30, 2020 09:00

adamraine approved these changes Nov 30, 2020

View reviewed changes

brendankenny reviewed Nov 30, 2020

View reviewed changes

apply comments

be29b12

vercel bot deployed to Preview December 5, 2020 05:11 View deployment

koh110 requested a review from brendankenny December 7, 2020 10:16

brendankenny approved these changes Dec 11, 2020

View reviewed changes

lighthouse-core/config/config-helpers.js Outdated Show resolved Hide resolved

s/relativePath/configDir

6d47db0

vercel bot deployed to Preview December 11, 2020 21:16 View deployment

brendankenny changed the title ~~core(config): load plugins from node_modules/~~ core: support local plugins from global Lighthouse Dec 11, 2020

brendankenny added the land-when-ci-is-green label Dec 11, 2020

devtools-bot merged commit 70106be into GoogleChrome:master Dec 11, 2020

devtools-bot removed the land-when-ci-is-green label Dec 11, 2020

koh110 deleted the plugin branch December 12, 2020 15:13

brendankenny mentioned this pull request Apr 15, 2021

Unclear model for installing/using a plugin alongside LH #9052

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: support local plugins from global Lighthouse #11696

core: support local plugins from global Lighthouse #11696

koh110 commented Nov 22, 2020 •

edited

Loading

adamraine Nov 23, 2020

connorjclark Nov 24, 2020

koh110 Nov 24, 2020 •

edited

Loading

connorjclark Nov 24, 2020 •

edited

Loading

koh110 Nov 24, 2020

connorjclark Nov 24, 2020

koh110 Nov 24, 2020

connorjclark Nov 24, 2020 •

edited

Loading

connorjclark Nov 25, 2020

adamraine Nov 25, 2020

connorjclark Nov 26, 2020

koh110 Nov 28, 2020

adamraine left a comment

brendankenny left a comment

brendankenny Nov 30, 2020

koh110 Dec 5, 2020

brendankenny Nov 30, 2020

brendankenny Nov 30, 2020

brendankenny Nov 30, 2020

brendankenny Nov 30, 2020

brendankenny Nov 30, 2020

brendankenny Nov 30, 2020

brendankenny Nov 30, 2020

brendankenny Nov 30, 2020

koh110 Dec 5, 2020

koh110 commented Dec 10, 2020

brendankenny left a comment

brendankenny commented Dec 11, 2020

koh110 commented Dec 12, 2020 •

edited

Loading

paulirish commented Dec 14, 2020

koh110 commented Dec 14, 2020

		!lighthouse-core/test/fixtures/config-helpers/config/node_modules
		!lighthouse-core/test/fixtures/config-helpers/node_modules

	// \| current working directory \| 2. \| 1. \|
	// \| in current working directory \| 2. \| 1. \|

	// Lighthouse globally installed, node_modules/ in current working directoriy.
	// Lighthouse globally installed, node_modules/ in current working directory.

core: support local plugins from global Lighthouse #11696

core: support local plugins from global Lighthouse #11696

Conversation

koh110 commented Nov 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koh110 Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

connorjclark Nov 24, 2020 • edited Loading

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

connorjclark Nov 24, 2020 • edited Loading

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

adamraine left a comment

Choose a reason for hiding this comment

brendankenny 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koh110 commented Dec 10, 2020

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny commented Dec 11, 2020

koh110 commented Dec 12, 2020 • edited Loading

paulirish commented Dec 14, 2020

koh110 commented Dec 14, 2020

koh110 commented Nov 22, 2020 •

edited

Loading

koh110 Nov 24, 2020 •

edited

Loading

connorjclark Nov 24, 2020 •

edited

Loading

connorjclark Nov 24, 2020 •

edited

Loading

koh110 commented Dec 12, 2020 •

edited

Loading