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

docs(puppeteer.md): examples of combining the two #4408

Merged
merged 2 commits into from
Feb 9, 2018
Merged

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented Feb 1, 2018

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for jumping on this @ebidel :)

**Note**: https://github.com/GoogleChrome/lighthouse/issues/3837 tracks the discussion for making Lighthouse work in concert with Puppeteer.
Some things are possible today (login to a page using Puppeteer, audit it using Lighthouse) while others (A/B testing the perf of UI changes) are not.

### Custom network throttle settings using Puppeteer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we advertise a different example of using puppeteer? Perhaps just logging in to your page with the second example you've already got?

Our preferred method of customizing throttling is already documented and jumping in to muck with those settings in puppeteer is bound to result in over confidence in the accuracy of the performance metrics for those conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that's fair. I can cook up a slightly different example of doing something in pptr then use lighthouse.

Copy link

@pedro93 pedro93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably make sense to showcase an example of interacting with a page such that it’s state changes (e.g: login) before executing lighthouse on it.

Puppeteer can reconnect to this existing browser instance like so:

```js
onst chromeLauncher = require('chrome-launcher');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Const typo

@ebidel
Copy link
Contributor Author

ebidel commented Feb 3, 2018

Changed to an example that injects css. As noted, it uses the protocol api instead of the pptr API to do so, but we can update the snippet when that starts working. I think the important bit for this doc is to show how to use puppeteer w/ LH.

Login is another example that I can add later.

@wardpeet
Copy link
Collaborator

wardpeet commented Feb 5, 2018

does it make sense to indent the async functions? not that it really matters but I just like that more 🙄

Thanks a lot @ebidel!

@ebidel
Copy link
Contributor Author

ebidel commented Feb 5, 2018

I usually don't b/c it creates unnecessary indentation. But happy to if that's what people want.

Flow:
1. disable the throttling settings in Lighthouse.
2. Launch Chrome using Puppeteer and tell Lighthouse to reuse Puppeteer's chrome instance.
3. Tell lighthouse to programmatically load the page.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Lighthouse' rather than 'lighthouse' I think...

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work on these. They are indeed the best existing solutions to the problem.

@paulirish paulirish merged commit 3e34af7 into master Feb 9, 2018
@paulirish paulirish deleted the ebidel-patch-3 branch February 9, 2018 15:17
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.

7 participants