Cookies #314
Cookies #314
Conversation
docs/api.md
Outdated
| If URLs are specified, only cookies for those URLs are returned. | ||
|
|
||
| #### page.deleteCookie(...cookies) | ||
| - `...cookies` <[Object]> |
There was a problem hiding this comment.
here and below: <[Array]<[Object]>>
There was a problem hiding this comment.
I don't like putting array when we have a spread operator. The user doesn't pass in an array.
|
An update to the cookie protocol is landing in r495262 https://chromium-review.googlesource.com/c/617750 |
docs/api.md
Outdated
| - `secure` <[boolean]> | ||
| - `sameSite` <[string]> `"Strict"` or `"Lax"`. | ||
|
|
||
| If no URLs are specified, this method returns cookies related to the current page. |
There was a problem hiding this comment.
I don't think you should inherit this poor DTP semantics. There is no real 'related to the page', there are cookies matching domains, paths and names. I would suggest to add optional name, domain and path parameters that would match the cookies. Your underlying implementation would be fetching everything using Network.getAllCookies.
There was a problem hiding this comment.
I'd agree if the method was browser.setCookie. But page.setCookie should act like I set a cookie for the page.
There was a problem hiding this comment.
I don't think you know what Network.getCookies without urls does...
There was a problem hiding this comment.
Ok now I don't think I do either.
There was a problem hiding this comment.
I want to be able to do
page.setCookie({name, value});
page.deleteCookie({name, value});There was a problem hiding this comment.
Disregard that, I read this comment as being on setCookie.
There was a problem hiding this comment.
(I think your understanding of 'related to the page' is different from the protocol one. I'm fine with your understanding, but it needs to be clarified and implementation needs to be changed. Also, you might want to pull this into the frame and alias in page)
| #### page.deleteCookie(...cookies) | ||
| - `...cookies` <...[Object]> | ||
| - `name` <[string]> **required** | ||
| - `url` <[string]> |
There was a problem hiding this comment.
Either url or domain should be required for scoping. Also, this method will delete multiple cookies for each pattern, so might be worth calling method deleteCookies and parameters patterns
| - `...cookies` <...[Object]> | ||
| - `name` <[string]> **required** | ||
| - `value` <[string]> **required** | ||
| - `url` <[string]> |
There was a problem hiding this comment.
either url or domain required.
There was a problem hiding this comment.
It uses the page url if none is specified.
There was a problem hiding this comment.
Cookies are not associated with the page, they are associated with the document (frame).
JoelEinbinder
force-pushed
the
cookies
branch
3 times, most recently
from
0d27e1c to
350e0ff
Compare
lib/Page.js
Outdated
| */ | ||
| async deleteCookie(...cookies) { | ||
| for (const cookie of cookies) { | ||
| const item = {}; |
There was a problem hiding this comment.
const item = Object.assign({}, cookie);
test/test.js
Outdated
| value: '3' | ||
| }); | ||
| expect(await page.evaluate('document.cookie')).toBe('cookie1=1; cookie2=2; cookie3=3'); | ||
| page.deleteCookie({name: 'cookie2'}); |
There was a problem hiding this comment.
technically, you'd want to await it here
lib/Page.js
Outdated
| for (const cookie of cookies) { | ||
| const item = {}; | ||
| Object.assign(item, cookie); | ||
| const pageURL = this.url(); |
There was a problem hiding this comment.
nit: bring pageURL out of the loop
Closes #53