-
Notifications
You must be signed in to change notification settings - Fork 222
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
Update to Async BrowserContext
#1590
Conversation
1401f15
to
55f97f1
Compare
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.
This is one heck of a job. 👍
I found one example where we're missing an await
, but other than that I couldn't find anything.
Left a suggestion on the return types as well.
docs/sources/next/javascript-api/k6-experimental/browser/browsercontext/grantpermissions.md
Show resolved
Hide resolved
docs/sources/next/javascript-api/k6-experimental/browser/browsercontext/cookies.md
Show resolved
Hide resolved
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.
LGTM
docs/sources/next/javascript-api/k6-experimental/browser/browsercontext/close.md
Outdated
Show resolved
Hide resolved
docs/sources/next/javascript-api/k6-experimental/browser/browsercontext/pages.md
Outdated
Show resolved
Hide resolved
@@ -5,14 +5,20 @@ description: 'Clears context cookies.' | |||
|
|||
# addCookies() | |||
|
|||
Adds a list of [cookies](https://grafana.com/docs/k6/<K6_VERSION>/javascript-api/k6-experimental/browser/browsercontext/cookie) into the [BrowserContext](https://grafana.com/docs/k6/<K6_VERSION>/javascript-api/k6-experimental/browser/browsercontext/cookie). All pages within this [BrowserContext](https://grafana.com/docs/k6/<K6_VERSION>/javascript-api/k6-experimental/browser/browsercontext/cookie) will have these [cookies](https://grafana.com/docs/k6/<K6_VERSION>/javascript-api/k6-experimental/browser/browsercontext/cookie) set. | |||
Adds a list of [cookies](https://grafana.com/docs/k6/<K6_VERSION>/javascript-api/k6-experimental/browser/browsercontext/cookie) into the [browser context](https://grafana.com/docs/k6/<K6_VERSION>/javascript-api/k6-experimental/browser/browsercontext/). All pages within this [browser context](https://grafana.com/docs/k6/<K6_VERSION>/javascript-api/k6-experimental/browser/browsercontext/) will have these [cookies](https://grafana.com/docs/k6/<K6_VERSION>/javascript-api/k6-experimental/browser/browsercontext/cookie) set. |
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'm not sure about the BrowserContext
to browser context
change. I think it makes it easier for the user to see it used in one way, so that they can easily understand the relationship between all the components.
Why did you make the change?
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.
It was easier to read in a sentence. We've been using this pattern in other places (before this PR), too. I've created an issue to keep everything consistent: #1596.
What?
Update relevant
BrowserContext
methods from sync to async.Checklist
npm start
command locally and verified that the changes look good.docs/sources/next
folder of the documentation.docs/sources/v{most_recent_release}
folder of the documentation.Related PR(s)/Issue(s)
grafana/xk6-browser#1295