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

Retrieve all cookies #1005

Merged
merged 15 commits into from
Aug 25, 2023
Merged

Retrieve all cookies #1005

merged 15 commits into from
Aug 25, 2023

Conversation

inancgumus
Copy link
Member

@inancgumus inancgumus commented Aug 21, 2023

What?

Adds browserContext.cookies() support.

There will be another PR to add browserContext.cookies([urls]) support to filter by cookie URLs.

Why?

  1. To let users access cookies.
  2. Providing a better Playwright compatibility.

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

The docs will be updated when #6 finishes.

Related PR(s)/Issue(s)

Updates #6

@inancgumus inancgumus self-assigned this Aug 21, 2023
@inancgumus inancgumus changed the title Add/browser context cookies Add browserContext.cookies([url]) Aug 21, 2023
@inancgumus inancgumus force-pushed the add/browser-context-cookies branch 2 times, most recently from f88d20b to 87f2812 Compare August 22, 2023 10:47
@inancgumus inancgumus force-pushed the add/browser-context-cookies branch 4 times, most recently from 3bc107f to d85fffe Compare August 22, 2023 13:14
@inancgumus inancgumus mentioned this pull request Aug 22, 2023
2 tasks
@inancgumus inancgumus force-pushed the add/browser-context-cookies branch from d85fffe to 2e06da8 Compare August 23, 2023 09:05
@inancgumus inancgumus changed the title Add browserContext.cookies([url]) Add browserContext.cookies() Aug 23, 2023
@inancgumus inancgumus marked this pull request as ready for review August 23, 2023 09:11
@inancgumus inancgumus requested review from ankur22 and ka3de August 23, 2023 09:12
@inancgumus inancgumus force-pushed the add/browser-context-cookies branch from 2e06da8 to 098b7ef Compare August 23, 2023 09:20
We can use this type in our code and tests and pass to clients. The
network.Cookie contains an internal fields and we don't want to use it.

This type provides type safety and representation. Instead of a map type
which its fields are of the same type, not type-safe, and not useful or
effective for representing a concept like a cookie.

Declaring this in the api package. So we can have less verbosity in the
rest of the codebase. The downside is the api package dependency.
If we declared this in the common package, there would be circular
dependency issues.
We take the cookies from the browser using CDP and then converting them
to our own cookie format: api.Cookie.
@inancgumus inancgumus force-pushed the add/browser-context-cookies branch from 3a8b73a to 7add7d1 Compare August 24, 2023 09:19
ka3de
ka3de previously approved these changes Aug 24, 2023
Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

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

This is great! 👏 I was just wondering where is the URLs filter, but I see this is just a part of #6.
LGTM.

tests/browser_context_test.go Show resolved Hide resolved
examples/cookies.js Outdated Show resolved Hide resolved
ankur22
ankur22 previously approved these changes Aug 24, 2023
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

LGTM 👏 Nicely done 🙂

I only left minor comments for you to consider 👍

tests/browser_context_test.go Show resolved Hide resolved
tests/browser_context_test.go Show resolved Hide resolved
tests/browser_context_test.go Show resolved Hide resolved
@inancgumus inancgumus dismissed stale reviews from ankur22 and ka3de via b4eeca4 August 24, 2023 15:33
@inancgumus inancgumus requested review from ka3de and ankur22 August 24, 2023 15:37
Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

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

LGTM.

@inancgumus inancgumus merged commit 6b7066c into main Aug 25, 2023
@inancgumus inancgumus deleted the add/browser-context-cookies branch August 25, 2023 07:16
@inancgumus inancgumus added the feature A new feature label Sep 7, 2023
@inancgumus inancgumus changed the title Add browserContext.cookies() Retrieve all cookies Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cookies feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants