-
Notifications
You must be signed in to change notification settings - Fork 27
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
Support modifying cookies #1384
Conversation
❌ Deploy Preview for content-scope-scripts failed.
|
Temporary Branch UpdateThe temporary branch has been updated with the latest changes. Below are the details:
Please use the above install command to update to the latest version. |
[Beta] Generated file diffTime updated: Tue, 14 Jan 2025 17:06:05 GMT Android
File has changed Integration
File has changed Apple
File has changed |
return [ | ||
{ name: 'specified cookie entry deleted', result: specifiedKey, expected: false }, | ||
{ name: 'specified cookie entry that is not present on page load', result: nonexistentKey, expected: false }, | ||
{ name: 'other cookie entry untouched', result: otherKey, expected: true } |
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.
Should we add a test that confirms path
is working as expected?
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.
Added test to check both states with path. I'd rather not add a second sub path test to validate some of the other cases (I don't think we need to validate the visibility of the cookie for example, because the behavior of cookie is path not set when deleting won't delete).
The same is true for domain but that's a bit more problematic as we intend to run these in a few places.
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.
Changes look good to me, everything seems to be working as expected. Left a couple comments here and one on the corresponding config PR.
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.
Thanks for adding the test, LGTM 👍
Asana Task/Github Issue: https://app.asana.com/0/1206777341262243/1209124294701647/f
Description
Support deletion of cookies for use in breakage situations.
Testing Steps
document.cookie="keyToBeDeleted=1"
in the consoleChecklist
Please tick all that apply: