-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs(plugins): update recipe example's lighthouse peer dependency version #9653
Conversation
update to lighthouse version that supports plugins
cc\ @brendankenny |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @alabiaga!
Co-Authored-By: Patrick Hulce <[email protected]>
Co-Authored-By: Patrick Hulce <[email protected]>
1. Install the plugin as a (peer) dependency, parallel to `lighthouse`. | ||
1. Install `lighthouse` v5 or later. | ||
2. Install the plugin as a (peer) dependency, parallel to `lighthouse`. | ||
3. Run `npx lighthouse lighthouse https://example.com --plugins=lighthouse-plugin-example --view` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm so trying the npx -p lighthouse
method it seems to force install lighthouse everytime which is not what we want
on [email protected]
running the original npx lighthouse https://example.com
seems to work just fine. @connorjclark are you able to still reproduce that bug you filed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on
[email protected]
running the originalnpx lighthouse https://example.com
seems to work just fine. @connorjclark are you able to still reproduce that bug you filed?
hmm, on [email protected]
I am getting the problems. It does sort of run lighthouse, but if you include flags (some flags?) it messes things up.
@patrickhulce are you able to run npx lighthouse https://example.com --plugins=lighthouse-plugin-not-a-real-plugin
and get a Runtime error encountered: Unable to locate plugin
message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brendankenny yup!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well I would say it's a bug introduced in a later version of npx but @connorjclark's original report was for 6.4.1 as well...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't reading the context of this PR just answering the npx question :) yeeea if we expect lighthouse to be installed in the folder then npx lighthouse ...
is the way to go.
Unfortunate that there's this bifurcation in behavior ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the end result of this?
The npx lighthouse lighthouse ...
call in the current commit doesn't work for me (it runs the same as the old npx lighthouse
) but npx -p lighthouse lighthouse...
does work. It seems like @connorjclark your advice to not check in a folder with lighthouse installed in it is to replicate the broken behavior...for the -p
version it should work whether it's installed locally or not :)
I say we go with the -p
since it works and at worst it just reinstalls lighthouse...but if you're really running a plugin (not just trying out an example plugin) you likely already have a way to run lighthouse (global install, local call within the repo, etc) and so won't be using npx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the -p lighthouse lighthouse
format sgtm. afaict the downside is that it always bypasses the local project's defined package versions and installation, and links the package and its deps in a tmp folder for every run. I killed my connection and verified it at least uses npm's global cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I'm pretty sure npx lighthouse lighthouse
is just a typo from copying my suggestion of npx -p lighthouse lighthouse
so I don't think anyone wants that one :) 👍
I really dislike reinstalling every time and npx -p lighthouse lighthouse <url>
when run in a project that has lighthouse and lighthouse-plugin-foo installed does not recognize and use the locally available plugin on 6.4.1
. I'm assuming because it's doing some global reinstall and not using the local build. Since this is the context of the guide here, I would strongly prefer to not give out a known broken command. At this point I would just rather list both, npx
does more harm than good
./node_modules/.bin/lighthouse --plugins=...
or
npm install -g lighthouse lighthouse-plugin-...
lighthouse
:( I wish it weren't broken but that just seems worse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for not being responsive as I've been working on the actual development of my plugin. So is the preferred documentation here just to use
./node_modules/.bin/lighthouse --plugins=...
Co-Authored-By: Patrick Hulce <[email protected]>
I started looking into this and think we have bigger problems to make new plugin dev straightforward and easy. I'll followup with a PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the meantime, these changes seem a slight improvement.
thanks all. |
Summary
Related Issues/PRs
Default selection for bin entry point is unexpected zkat/npx#215 captures the issue with the npx command