Navigation Menu

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(auth): add setCookie example #11473

Merged
merged 3 commits into from Sep 29, 2020
Merged

docs(auth): add setCookie example #11473

merged 3 commits into from Sep 29, 2020

Conversation

patrickhulce
Copy link
Collaborator

Summary
Promised followup to #9170, adds a setCookie example and updates some of our pptr docs. @paulirish's prior PR #10277 did some of this already, so just a few more minor tweaks.

Related Issues/PRs
closes #11470

@patrickhulce patrickhulce requested a review from a team as a code owner September 22, 2020 15:46
@patrickhulce patrickhulce requested review from brendankenny and removed request for a team September 22, 2020 15:46

See [a working demo at /docs/recipes/auth](./recipes/auth).

View our full documentation for using [Lighthouse along with Puppeteer](https://github.com/GoogleChrome/lighthouse/blob/master/docs/puppeteer.md).

You may want to use Puppeteer's [`page.setCookie`](https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#pagesetcookiecookies).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we don't really want to encourage anyone do to this

@@ -32,7 +30,7 @@ const result = await lighthouse('http://www.example.com', {
});
```

You could also set the `Cookie` header, but beware: it will [override any other Cookies you expect to be there](https://github.com/GoogleChrome/lighthouse/pull/9170).
You could also set the `Cookie` header, but beware: it will [override any other Cookies you expect to be there](https://github.com/GoogleChrome/lighthouse/pull/9170). For a more flexible cookie-based approach, use [puppeteer (Option 1)](./recipes/auth/README.md) instead.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nudge them toward option 1 so they don't end up asking for #9170 all over again :)

@@ -1,5 +1,13 @@
# Using Puppeteer with Lighthouse

## Recipes

### [Using Puppeteer for authenticated pages](./recipes/auth/README.md)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this seemed much more important to place at the top than injecting stylesheets

if (page && page.url() === url) {
// Note: can't use page.addStyleTag due to github.com/GoogleChrome/puppeteer/issues/1955.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wasn't true anymore

@@ -19,32 +27,16 @@ const browser = await puppeteer.launch({
defaultViewport: null,
});

// Wait for Lighthouse to open url, then customize network conditions.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

comments were describing something that wasn't happening...

docs/recipes/auth/README.md Outdated Show resolved Hide resolved
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -1,5 +1,13 @@
# Using Puppeteer with Lighthouse

## Recipes

### [Using Puppeteer for authenticated pages](./recipes/auth/README.md)
Copy link
Member

Choose a reason for hiding this comment

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

would these be better as bullet points or something? IMO they look kind of weird as linked headings with no content, but the formatting is also not terribly important.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no strong preference, I was just matching the style used for the custom gatherer one, and I see the value of bumping their visual prominence a bit over regular text-sized bullets.

Do they look weird enough to you to change? :) I'll accept whatever suggestion you put up

Copy link
Member

Choose a reason for hiding this comment

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

I see the value of bumping their visual prominence a bit over regular text-sized bullets.

Do they look weird enough to you to change? :) I'll accept whatever suggestion you put up

No suggestion, sorry, and I figured that was the reasoning. It is a little awkward looking but it's also functional and clear in what that function is, so that's why I said not terribly important :)

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

Successfully merging this pull request may close these issues.

Add page.setCookie documentation
4 participants