Skip to content
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

cli(yarn): Add mixed-content yarn script #4344

Merged
merged 1 commit into from
Jan 26, 2018

Conversation

christhompson
Copy link
Contributor

This adds a new yarn script (as discussed in #3953) which simplifies running the custom configuration required for the mixed-content audit.

@paulirish Could you PTAL to see if this is what you had in mind?

@christhompson
Copy link
Contributor Author

FYI I don't have write access to merge this, so if this looks good to go could a core member merge it? Thanks!

@patrickhulce patrickhulce merged commit c225d01 into GoogleChrome:master Jan 26, 2018
@paulirish
Copy link
Member

As for someone consuming this after a global install of LH... i guess they would do:

npm explore lighthouse -- npm run mixed-content

@paulirish
Copy link
Member

@christhompson I published a prerelease so we can test out this flow:

npm install --global lighthouse@canary
npm explore --global lighthouse -- npm run mixed-content http://bbc.com

This works for me, though it saves the result in a funny spot (because of the explore call resetting CWD, i guess):

Printer html output written to /Users/paulirish/.homebrew/lib/node_modules/lighthouse/www.bbc.com_2018-02-06_12-46-36.report.html

But aside from that it seems roughly fine.

@christhompson
Copy link
Contributor Author

Thanks for publishing the canary release -- that will help with testing the final documentation.

The working-dir issue seems unfortunate but maybe okay overall. It might be best if the example invocation we give includes --view to automatically open it. The concern then would be that it is storing them in a directory where the user would have trouble clearing them out.

It might work better to recommend the flow, which would leave the user's shell in the lighthouse directory after running?

npm install --global lighthouse
npm explore lighthouse
npm run mixed-content http://www.bbc.com

@paulirish
Copy link
Member

@christhompson hmmm that sounds cool, but it's not working out so hot on my machine:

image

i'm gonna see if i can trick LH into saving the file locally.

@paulirish
Copy link
Member

okay got a solution:

npm explore --global lighthouse -- npm run mixed-content -- --output-path=$PWD http://bbc.com

this ALMOST works and will work once #4369 is landed. but still, the experience here is pretty ugly.

maybe a new commandline flag --mixed-content is the right answer?

@christhompson
Copy link
Contributor Author

If you're okay with using flags for this, it would definitely make invoking it a lot simpler. I don't want to make precedent for adding a laundry list of flags though.

I've uploaded a quick PR for adding a flag to do this, but there may a cleaner way to handle the alternate config than what I did in lighthouse-core/index.js. See #4441

@patrickhulce
Copy link
Collaborator

I don't want to make precedent for adding a laundry list of flags though.

We've already added rethinking how we invoke non-default configs to our thoughts for #4333 on next breaking change, so if you're cool with this fix in 2.x releases then sgtm :)

@christhompson
Copy link
Contributor Author

sgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants