core(plugins): resolve plugins and audits from cwd #9950

paulirish · 2019-11-09T07:00:18Z

This is an alternative to #9948

"Your colleagues will hate you for it!"

my goals here

plugin developers have a sane flow
plugin users have zero surprises or anything odd.

and two things that are explicitly not optimized:

lighthouse core developers don't have a straightforward mechanism of developing plugins within the core repo
plugin developers who also have a lighthouse checkout may face weirdness if they want to hack on lighthouse at the same time as the plugin.

connorjclark · 2019-11-09T08:01:22Z

no hate 💓

@paulirish's approach only considers plugin authors being able to run their own plugin super easily. Call it focused 🤓

My approach allows for the same, but also supports using a plugin from anywhere on the filesystem. Call it flexible 🏃‍♂️. This is possible with the other approach, but requires usage of yarn link, a tool I find to be low in DX (how do you see what packages are currently linked? hint: it's not supported).

No doubt, the focused 🤓 approach is less code and fixes the largest pain point we know about for plugin development. I think it's probably what we should go with. But I'll point out the differences anyway.

common supported usecase: run my plugin under development

Note that it is assumed that lighthouse is installed as a dependency/devDependency.

focused 🤓

# be in their plugin repo

# run
yarn lighthouse https://www.example.com --plugins=. --only-categories=.

# run just their plugin
yarn lighthouse https://www.example.com --plugins=. --only-categories=.

flexible 🏃‍♂️

# be in their plugin repo

# run
yarn lighthouse --plugins=.

# run just their plugin
yarn lighthouse --plugins=. --only-categories=lighthouse-plugin-yt
yarn lighthouse --plugins=. --only-categories=`basename "$PWD"`
# would like to support `.` but I didn't special case it yet

using any plugin

flexible 🏃‍♂️

# 1) global installation (could also be local checkout `node lighthouse-cli ...`)
# for some reason I am a user with plugins but don't want to publish them to npm. that's okay because I can just point lighthouse to my local copy of it
lighthouse --plugins=~/code/lighthouse-plugins/lighthouse-plugin-yt

# 2) back in a plugin repo, but imagine it is a plugin monorepo because I have a lot of opinions and need many lighthouse plugins
yarn lighthouse --plugins=./plugins/lighthouse-plugin-trending
yarn lighthouse --plugins=./plugins/lighthouse-plugin-voice

# 3) run both
yarn lighthouse --plugins=./plugins/lighthouse-plugin-trending,./plugins/lighthouse-plugin-voice
# ...

focused 🤓

# 1) can do the same thing but I need to cd to the folder first, or use yarn link
cd ~/code/lighthouse-plugins/lighthouse-plugin-yt
yarn lighthouse https://www.example.com --plugins=. --only-categories=.

# 2) can do the same thing but I need to cd to the folder first, or use yarn link
cd plugins/lighthouse-plugin-trending && yarn lighthouse https://www.example.com --plugins=. --only-categories=.
cd plugins/lighthouse-plugin-voice && yarn lighthouse https://www.example.com --plugins=. --only-categories=.


# 3) can't do the same thing without some yarn links

possible shorthand

Filtering to just the plugin whose directory you are currently in is stranger in my approach, but I can think of a third approach that's better than both (from the perspective of a plugin author). Something like --debug-local-plugin as a shorthand for the options above (it would just set --plugins and only-categories for you).

connorjclark · 2019-11-09T21:06:40Z

this works with none of our changes:

# be in plugin directory
NODE_PATH=.. yarn lighthouse https://paulirish.github.io/lite-youtube-embed/ --plugins=lighthouse-plugin-yt --only-categories=lighthouse-plugin-yt

brendankenny

I'm definitely a fan of the simpler approach for now. I don't know if we want to encourage plugin use outside of the module system, but we obviously do need to improve plugin development ergonomics

brendankenny · 2019-11-09T20:01:16Z

docs/recipes/lighthouse-plugin-example/package.json

@@ -4,5 +4,8 @@
  "main": "./plugin.js",
  "peerDependencies": {
    "lighthouse": "^5.0.0"
+  },
+  "devDependencies": {
+    "lighthouse": "latest"


this could cause problems, though it sounds like y'all have talked this through? If the version of LH that the plugin depends on is different than the one running the plugin, weird things happen with e.g. ICU, logging, sentry, the report generator, etc. At least as a devDependency it'll only be during plugin development.

nah it should be a more specific version range

lighthouse-cli/cli-flags.js

brendankenny · 2019-11-09T20:25:52Z

lighthouse-core/config/config.js

@@ -161,6 +161,9 @@ function assertValidGatherer(gathererInstance, gathererName) {
 * @param {string} pluginName
 */
 function assertValidPluginName(configJSON, pluginName) {
+  // For local plugin development
+  if (pluginName === '.') return;


what about

Suggested change

if (pluginName === '.') return;

if (pluginName === '.') {

pluginName = path.basename(process.cwd());

}

to make sure we don't encourage plugins not named lighthouse-plugin- whatever? (I guess the most correct thing would be to read from package.json, but I don't know how much of a pain that would be)

or do the basename thing in bin.js...maybe we don't want folks testing with . as a category? It makes for some weird LHR differences than when running a plugin as a user

i like reading the real thing from the folder, yeah.

brendankenny · 2019-11-09T20:35:10Z

lighthouse-core/config/config-helpers.js

+  if (moduleIdentifier.startsWith('lighthouse-plugin-')) {
+    // See if the module resolves in the context of the current working directory.
+    // Most useful to handle the case of Lighthouse plugin development.
+    const cwdPath = path.resolve(process.cwd(), '../', moduleIdentifier);


if we don't like this, the doubled up lighthouse-plugin-whatever could be taken care of in config-plugin.js. It we passed in the resolved plugin path, we could then rewrite the audit paths as absolute in _parseAuditsList (or relative to some known entity, like the lh root or cwd or whatever).

It should allow getting rid of making plugins prefix their audits with the plugin name (and it could allow relative paths to any plugin).

Not sure if we want to do any of that, though.

yah that's a good idea.

IMO the audit path that we expect in plugin.js doesn't make sense to me. eg.

https://github.com/connorjclark/lighthouse-plugin-yt/blob/136869d31885b3894d12882350989c8c9112c4b6/plugin.js#L6

wdyt about for v6 we would change that so that the audit paths defined in plugin.js are relative

oh wait. i think that'd simplify plugin dev but it'd break plugin use? seems like it would in theory.

@connorjclark @brendankenny does that sound right? that we essentially need the plugin-name in the path so the resolution for a user works?

We can do this change w/o breaking plugin users. Just need to try one more path in our resolution strategy (prefix the name of the plugin to the audit module path, if the initial resolution didnt work)

Co-Authored-By: Brendan Kenny <[email protected]>

connorjclark · 2019-11-09T22:50:06Z

my vote is to close both of these PRs and just update our plugin docs with:

#9950 (comment)

paulirish · 2019-11-09T22:51:05Z

this works with none of our changes:

# be in plugin directory
NODE_PATH=.. yarn lighthouse https://paulirish.github.io/lite-youtube-embed/ --plugins=lighthouse-plugin-yt --only-categories=lighthouse-plugin-yt

this sounds pretty cool, though i am scared. it's quite simple.

connorjclark · 2019-11-09T22:58:46Z

even this works (running from lighthouse dir, master)

NODE_PATH=~/code/ node lighthouse-cli/ https://www.example.com --plugins=lighthouse-plugin-yt --only-categories=lighthouse-plugin-yt  --view

I feel like I gained a super power

paulirish added 5 commits September 16, 2019 17:29

fix plugins by using recipe as a template.

7fac0d7

Merge branch 'master' into pluginfixes

ca44548

easier module resolution for plugins

d0aaad5

readme dev hints

1928f21

cli flags from 9948, though tweaked.

e977e22

paulirish requested review from brendankenny and patrickhulce as code owners November 9, 2019 07:00

googlebot added the cla: yes label Nov 9, 2019

brendankenny reviewed Nov 9, 2019

View reviewed changes

Update lighthouse-cli/cli-flags.js

d1e73ae

Co-Authored-By: Brendan Kenny <[email protected]>

vercel bot deployed to staging November 9, 2019 22:40 View deployment

paulirish mentioned this pull request Nov 25, 2019

docs(plugins): update plugin developer docs wrt NODE_PATH #10028

Merged

paulirish closed this in #10028 Dec 4, 2019

paulirish deleted the pluginfixes branch December 4, 2019 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(plugins): resolve plugins and audits from cwd #9950

core(plugins): resolve plugins and audits from cwd #9950

paulirish commented Nov 9, 2019 •

edited

Loading

connorjclark commented Nov 9, 2019

connorjclark commented Nov 9, 2019 •

edited

Loading

brendankenny left a comment

brendankenny Nov 9, 2019

paulirish Nov 9, 2019

brendankenny Nov 9, 2019

brendankenny Nov 9, 2019

paulirish Nov 9, 2019

brendankenny Nov 9, 2019

paulirish Nov 9, 2019

connorjclark Nov 9, 2019

connorjclark commented Nov 9, 2019

paulirish commented Nov 9, 2019

connorjclark commented Nov 9, 2019 •

edited

Loading

-  if (pluginName === '.') return;
+  if (pluginName === '.') {
+    pluginName = path.basename(process.cwd());
+  }

core(plugins): resolve plugins and audits from cwd #9950

core(plugins): resolve plugins and audits from cwd #9950

Conversation

paulirish commented Nov 9, 2019 • edited Loading

connorjclark commented Nov 9, 2019

common supported usecase: run my plugin under development

using any plugin

possible shorthand

connorjclark commented Nov 9, 2019 • edited Loading

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

connorjclark commented Nov 9, 2019

paulirish commented Nov 9, 2019

connorjclark commented Nov 9, 2019 • edited Loading

paulirish commented Nov 9, 2019 •

edited

Loading

connorjclark commented Nov 9, 2019 •

edited

Loading

connorjclark commented Nov 9, 2019 •

edited

Loading