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

Possible API Sketch #14

Closed
esprehn opened this issue May 3, 2016 · 10 comments
Closed

Possible API Sketch #14

esprehn opened this issue May 3, 2016 · 10 comments

Comments

@esprehn
Copy link

esprehn commented May 3, 2016

interface Cookie {
    readonly DOMString name;
    readonly DOMString value;
    readonly DOMString path;
};

interface CookieList {
    maplike<DOMString, sequence<Cookie>>;
    Cookie get(DOMString name);
    squence<Cookie> getAll(DOMString name);
};

dictionary CookieOptions {
    DOMString path;
    (DOMString or Date) expiration;
};

interface CookieStore extends EventTarget {
    Promise<CookieList> get(optional DOMString name);
    Promise<CookieList> getAll();
    Promise<CookieList> match((DOMString or Regexp) name, (DOMString or Regexp) path);
    Promise<CookieList> matchAll((DOMString or Regexp) name, (DOMString or Regexp) path);
    Promise<void> put(DOMString name, DOMString value, optional CookieOptions);
};

document.cookieStore.addEventListener("change", function(event) {
    // event.detail == CookieList
});

partial interface Document {
    CookieStore cookieStore;
};

partial interface ServiceWorkerGlobalScope {
    CookieStore cookieStore;
};

partial interface InstallEvent {
  void registerCookieInterest(...);
};
@bsittler
Copy link
Contributor

bsittler commented May 3, 2016

What do you think, @domenic and @dmurph ? It would give us a way to register interest in ServiceWorker contexts but not depend on observers

@domenic
Copy link

domenic commented May 3, 2016

I don't understand what problem registerCookieInterest is trying to solve?

It shouldn't be Document + ServiceWorkerGlobalScope. Just do WindowOrWorkerGlobalScope.

get() and match() should return Cookies, not CookieLists?

If there's no distinction between adding and overwriting, just use set() as the name. Cache API uses add() + put() to distinguish but in this case there doesn't seem to be such a distinction.

@domenic
Copy link

domenic commented May 3, 2016

event.detail could instead be an array of names, perhaps?

@mkruisselbrink
Copy link
Collaborator

match and matchAll should probably take some kind of CookieMatchOptions dictionary with options for which cookies to return, rather than having name and path parameters. And I assume that same dictionary would also be passed to registerCookieInterest?

@mkruisselbrink
Copy link
Collaborator

Also using a non-top-level event for this from service workers would diverge a bit from how other service worker events are dealt with. But maybe that isn't a bad thing. We would need to consider how we would extend/modify the currently specced behavior around recording which events have event listeners installed and only waking up the worker for those events.

@annevk
Copy link
Collaborator

annevk commented May 4, 2016

Cookies are utf-8, right? Should use USVString.

@dmurph
Copy link

dmurph commented May 4, 2016

First, are the cookies in the change event all existing ones? or just the changed ones?

Second, do we need to have consistent state for cookie reading? As in, in my 'change' event for the cookie, should I be able to read all cookies? I don't think that people will want this, but someone might want to ability to read a consistent state of all the cookies in that change event.

You could do this by providing a copy of the current state in the event? Or make it an optional feature to include that? After all, we limit our cookie sizes to something not-too-crazy, right?

But I think it would also probably be fine to just include the new cookie values.

@bsittler
Copy link
Contributor

I've attempted to address some of these issues in the new explainer and I'd love to hear your thoughts on it, and would be happy to discuss and address any outstanding issues (perhaps in open pull request #17 ?)

The version there proposes that only cookies you have indicated interest in are included in the change event and provides no way to view other unrelated cookies at that point in time, however you can achieve the same effect by instead asking for all cookies when configuring the observer/ServiceWorker event and doing any needed filtering in your handler. In this case all script-visible cookies are returned in as consistent a state as that currently used when serializing the response for a document.cookie read. This is useful e.g. to simulate the script-visible subset of the Cookie: header a server would see.

I went ahead and mandated UTF-8 interpretation for cookie data and a USVString script interface but I don't know yet whether the currently non-UTF-8-cookie browsers (WinINet-based and Safari) will be able to adopt that as it represents a change in behavior.

I switched to a top-level event in the ServiceWorker case for consistency with existing events.

match and matchAll are gone in favor of get and getAll, and matching options are now exact-match (matchType 'equals') and exact prefix match (matchType 'startsWith').

set was chosen based on @domenic 's suggestion above (and other similar discussions.)

get returns a single cookie object now, getAll an array.

event.detail is now an array of cookie change entries, each with 'name' and 'type' (and 'value' too when the cookie has a value in its new state)

bsittler added a commit that referenced this issue Aug 12, 2016
@bsittler
Copy link
Contributor

The document-appropriate subset of the new API based on @esprehn 's proposal is now implemented in the polyfill and described in the explainer

@pwnall
Copy link
Collaborator

pwnall commented May 30, 2018

The explainer now reflects the latest proposal version implemented in Blink. https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/cookie_store/ contains IDL files.

@esprehn Thank you very much for the initial sketch! I think you'll find that we haven't drifted too far from it.

@pwnall pwnall closed this as completed May 30, 2018
pwnall added a commit that referenced this issue Feb 5, 2020
1) Fixes all instances of await being used in non-async functions.
2) Fixes unfinished sentences noticed along the way.

Fixes #14, Fixes #15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants