Skip to content
This repository was archived by the owner on Nov 24, 2018. It is now read-only.

Conversation

@joeyvandijk
Copy link
Contributor

cookiesClear did not check if it could be executed, so added that check, but also rewrote the API names to make it more clear what both endpoints do(n't).

@adieuadieu
Copy link
Collaborator

adieuadieu commented Aug 1, 2017

Just a note: changing the API method names would be considered a breaking change meaning a bump in the major version. It's probably OK to rename for now without a major bump, but this would be the exception. @joelgriffith @timsuchanek?

Furthermore, I'm wondering about naming convention. If we've adopted the setXXX and XXX convention, does it follow that it should be deleteXXX ?

cookiesDelete(name: string, url: string): Chromeless<T> {
if (typeof name === 'undefined') {
throw new Error('Cookie name should be defined.')
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably have this method lookup the URL since it'll likely be ran in the context of the page it's in. This is good for now, but might be a nice change/improvement later on!

@adieuadieu adieuadieu added this to the v1.2 milestone Aug 3, 2017
@joeyvandijk
Copy link
Contributor Author

@adieuadieu so you mean deleteCookies(name:string) and clearCookies(), but do I also need to change the set/get cookie functions or is that another PR?

@adieuadieu
Copy link
Collaborator

@joeyvandijk let's rename the others in another PR. Technically we're introducing breaking changes...

@joeyvandijk joeyvandijk changed the title rewrite cookie endpoints: cookiesDelete + cookiesClear and fix cookiesClear rewrite cookie endpoints: deleteCookies & clearCookies and fix clearCookies Aug 3, 2017
@adieuadieu adieuadieu removed the feature label Aug 3, 2017
Copy link
Collaborator

@adieuadieu adieuadieu left a comment

Choose a reason for hiding this comment

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

Thank you, @joeyvandijk!

@adieuadieu adieuadieu merged commit 3e84f0e into schickling:master Aug 3, 2017
@joeyvandijk
Copy link
Contributor Author

👍

@joeyvandijk joeyvandijk deleted the bugfix/can-cookieClearAll branch August 3, 2017 20:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants