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

Switch from get/set to read/write; add readAllAsString/writeAsString #9

Closed
wants to merge 3 commits into from

Conversation

bsittler
Copy link
Contributor

@bsittler bsittler commented Mar 4, 2016

No description provided.

@domenic
Copy link

domenic commented Mar 4, 2016

Wait, no, I liked get/set for the map-like methods and read/write for the string methods. That makes it clear how different they are.

Also the switch to arrow functions and property initializers is a bad idea as it's not a standard ES feature and it changes the semantics by generating a new method for each instance of the class.

@bsittler
Copy link
Contributor Author

bsittler commented Mar 4, 2016

Oh, I see. I switched verbs because I felt get/set implied synchronous operations whereas these use promises. What set of methods would you like to see, and with what shape for the resolved values?

@domenic
Copy link

domenic commented Mar 4, 2016

To me the following makes sense:

Promise<boolean> has(...)
Promise<string> get(...)
Promise<array<string>> getAll(...)
Promise<void> delete(...)
Promise<void> set(...)

Promise<string> readAsString()
Promise<void> writeAsString(s)

@domenic
Copy link

domenic commented Mar 4, 2016

There's some precedent for promise-returning has/delete at least in service worker's cache API, and promise-returning get in its clients API.

Also SyntaxError everywhere
@bsittler
Copy link
Contributor Author

bsittler commented Mar 4, 2016

Any guidance on method naming for "read the whole jar"?

@bsittler
Copy link
Contributor Author

bsittler commented Mar 4, 2016

Also, readAsString to me implies a single cookie, which is not the case - it's the whole jar.

@domenic
Copy link

domenic commented Mar 5, 2016

navigator.cookies.readAsString() taking no args seems to me like it reads the whole jar.

I'm not sure what "read the whole jar" means exactly if it's not returning a string. If it's something like the current implementation's .get() and .getAll(), I think those are honestly unnecessary; I'm not sure what their use cases are, at least.

@domenic
Copy link

domenic commented Mar 5, 2016

You could also consider serializeToString() and resetFromSerializedString(s). Kind of long, but it's not like they're the happy path methods that need nice simple names.

@bsittler
Copy link
Contributor Author

bsittler commented Mar 5, 2016

The list-of-objects-returning behavior is indeed a relic of
pre-read(All)AsString days. The map behavior, though, is not.

The idea was to make it easy to get the values of multiple cookies in a
single promise resolution. This is used, for example, in checking to see
whether there is a signed-in user session (call this cookie
SessionIdentifier) and computing an XSRF token by doing some math on the
value of a different cookie (call this other cookie
SessionStateFingerprint). Likewise if JS-mutable preferences are stored in
one cookie (SessionIdentifier again) and a session identifier for signed-in
user(s) in another (call this third cookie Preferences), whether the
preferences are applied (and which ones) may well be dependent upon the
presence or absence of the SessionIdentifier cookie. With document.cookie a
single read + parse can extract all the needed values to make these
determinations, and typically the cookies are put in a map.

On Fri, Mar 4, 2016 at 4:00 PM, Domenic Denicola [email protected]
wrote:

navigator.cookies.readAsString() taking no args seems to me like it reads
the whole jar.

I'm not sure what "read the whole jar" means exactly if it's not returning
a string. If it's something like the current implementation's .get() and
.getAll(), I think those are honestly unnecessary; I'm not sure what their
use cases are, at least.


Reply to this email directly or view it on GitHub
#9 (comment)
.

@bsittler
Copy link
Contributor Author

bsittler commented Mar 5, 2016

I'm concerned by that because to me it implies the strings are compatible;
in general, though, they are not.

On Fri, Mar 4, 2016 at 4:07 PM, Domenic Denicola [email protected]
wrote:

You could also consider serializeToString() and
resetFromSerializedString(s). Kind of long, but it's not like they're the
happy path methods that need nice simple names.


Reply to this email directly or view it on GitHub
#9 (comment)
.

@domenic
Copy link

domenic commented Mar 5, 2016

I'm concerned by that because to me it implies the strings are compatible; in general, though, they are not.

Hmm, I thought they were. You're saying doing document.cookie = document.cookie in general will change the cookie jar?

The idea was to make it easy to get the values of multiple cookies in a single promise resolution.

Is there a reason Promise.all is not sufficient here? Building out of compositional primitives makes more sense to me.

Remember, your polyfill is working with an opaque string, and so it has to parse every time. But browsers have an actual data structure they're exposing, and document.cookie actually serializes it upon access.

If you do need to go down this route, you're going to need to create a new InMemoryCookieJar synchronous multimap class (cf. URLSearchParams) that can be used as the resolution of the promise. You can't serialize it to a single Map or JavaScript object since it has multimap structure.

@bsittler
Copy link
Contributor Author

bsittler commented Mar 5, 2016

document.cookie = document.cookie does in general change the cookie jar. It interprets the read value (in Cookie: syntax) according to Set-Cookie: syntax. Only the first cookie has a chance of being preserved, and only then if all cookie "key" attributes (none of which are serialized in Cookie: nor in document.cookie reads) happen to line up with the defaults as modified by subsequent cookies misinterpreted as attributes. All read cookies after the first are misinterpreted as attributes (so e.g. a cookie named "expires" is interpreted as a datetime string controlling expiration).

For instance, in reading document.cookie the string

"Foo=Bar; Max-Age=37; Foo=Qux; Secure; HttpOnly"

represents the following five cookies:

  1. cookie "Foo" with value "Bar"; path, domain, secure, max-age, expiration unknown (but presumably in-scope and not expired); HttpOnly not set
  2. cookie "Max-Age" with value "37"; path, domain, secure, max-age, expiration unknown (but presumably in-scope and not expired); HttpOnly not set
  3. cookie "Foo" with value "Qux"; path, domain, secure, max-age, expiration unknown (but presumably in-scope and not expired, and definitely differing in some way from 1. and with scoping different such that the browser chooses to serialize it later than 1.); HttpOnly not set
  4. cookie "Secure" with no value (discouraged by RFC but browsers do it); path, domain, secure, max-age, expiration unknown (but presumably in-scope and not expired, and with scoping different such that the browser chooses to serialize it later than 1.); HttpOnly not set
  5. cookie "HttpOnly" with no value (discouraged by RFC but browsers do it); path, domain, secure, max-age, expiration unknown (but presumably in-scope and not expired, and with scoping different such that the browser chooses to serialize it later than 1.); HttpOnly not set

however, when the same string is written to document.cookie it represents a single cookie:

cookie "Foo" with value "Bar" and the following four attributes:

  1. Max-Age 37
  2. Ignored custom attribute "Foo" with value "Qux"
  3. Secure
  4. HttpOnly (so in fact it won't appear in a document.cookie read)
  5. Domain implicit (since it's Secure, this is essentially Origin-scoped modulo port number wierdness in some browsers)
  6. Path computed from the path of the current document

Also document.cookie writes (like Set-Cookie) can create cookies with out-of-scope paths (paths not visible in the current document's scope, e.g. Path=/some/other/path) and the cookies will be invisible to document.cookie but readable at the other path.

@bsittler
Copy link
Contributor Author

bsittler commented Mar 5, 2016

The order in the serialization matters, too. Relative ordering when duplicate cookie names are present indicates predictable, meaningful differences in cookie scope.

@bsittler
Copy link
Contributor Author

bsittler commented Mar 5, 2016

Frankly, the more I look at all this the more inclined I am to not model it as a map of any sort!

@bsittler
Copy link
Contributor Author

bsittler commented Mar 5, 2016

An interface more akin to https://closure-library.googlecode.com/git-history/docs/class_goog_net_Cookies.html might be preferable, in fact.

@bsittler
Copy link
Contributor Author

bsittler commented Mar 5, 2016

We may need transactions too @inexorabletash

@bsittler
Copy link
Contributor Author

bsittler commented Mar 5, 2016

(I say that because sets of interlinked session cookies are often all updated in a single HTTP response, and the usually-valid assumption is that a single Cookie header or document.cookie read reflects a consistent state)

@domenic
Copy link

domenic commented Mar 5, 2016

document.cookie = document.cookie does in general change the cookie jar.

Thanks for setting me straight. This does indicate a need for very divergent names; I agree. Hmm. Maybe setFromString(s) and readAllAsString() or serializeAllToString() is reasonable.

Frankly, the more I look at all this the more inclined I am to not model it as a map of any sort!

That seems like a shame. Legacy document.cookie API aside, I think the in-memory data structure we're trying to expose is exactly a multimap, isn't it?

An interface more akin to https://closure-library.googlecode.com/git-history/docs/class_goog_net_Cookies.html might be preferable, in fact.

This seems basically to be a map API, which is just a crippled subset of a multimap API.

The utility methods isValidName and isValidValue are a good idea though, although I'd make them statics instead of instance methods.

@bsittler
Copy link
Contributor Author

bsittler commented Mar 5, 2016

It's only deceptively similar to a map. It's true that the browser-internal representation is a map, but parts of the key are hidden in the read interface, as are some of the values (protocol-, origin- and path-dependently).

@bsittler
Copy link
Contributor Author

bsittler commented Mar 5, 2016

Also, the browser-internal store is definitely a map, not a multimap. Apparent multi-values in Cookie: and reads of document.cookie are due to the hidden parts of the key differing while the exposed component (name) matches.

Note that the proposed Origin cookies could be exposed as a straightforward map, but it's a separate one from regular cookies.

@domenic
Copy link

domenic commented Mar 5, 2016

Also, the browser-internal store is definitely a map, not a multimap. Apparent multi-values in Cookie: and reads of document.cookie are due to the hidden parts of the key differing while the exposed component (name) matches.

You can always emulate a multimap as a map with hidden keys. But I don't think introducing a new concept like "hidden keys" makes sense when you can just use an existing intuitive concept of "multimap"

Note that the proposed Origin cookies could be exposed as a straightforward map, but it's a separate one from regular cookies.

Is that true? I thought you were still allowed to set two cookies with the same name and read back both values. That is how the Set-Cookie header normally works.

@bsittler
Copy link
Contributor Author

bsittler commented Mar 5, 2016

Two cookies can never share the same domain+path+name. Instead, the second
such Set-Cookie:/document.cookie write overwrites the first (this is also
how cookies are deleted/cleared before expiration, by overwriting with an
expired cookie with a matching key triple.) So the full map key is visible
in Set-Cookie:, but only the name part is visible in the
Cookie:/document.cookie read serialization. Also, the position of the
serialized name=value pair in the serialization depends on the full key,
even though only the name part of that is serialized there.

Given that, I insist that it's a map, not a multimap. Does this make sense?
On Mar 4, 2016 10:21 PM, "Domenic Denicola" [email protected]
wrote:

Also, the browser-internal store is definitely a map, not a multimap.
Apparent multi-values in Cookie: and reads of document.cookie are due to
the hidden parts of the key differing while the exposed component (name)
matches.

You can always emulate a multimap as a map with hidden keys. But I don't
think introducing a new concept like "hidden keys" makes sense when you can
just use an existing intuitive concept of "multimap"

Note that the proposed Origin cookies could be exposed as a
straightforward map, but it's a separate one from regular cookies.

Is that true? I thought you were still allowed to set two cookies with the
same name and read back both values. That is how the Set-Cookie header
normally works.


Reply to this email directly or view it on GitHub
#9 (comment)
.

@domenic
Copy link

domenic commented Mar 7, 2016

I see. I think I was confused with the fact that headers are a multimap because of Set-Cookie. But the cookie store itself is indeed a map, it sounds like (keyed on a complex key, but still).

Thanks for setting me straight.

@bsittler
Copy link
Contributor Author

bsittler commented Mar 7, 2016

Given that it's a map with a complex key, and only a single (ambiguous) part of the key is directly usable in lookup operations, do you have any ideas on method naming for the asynchronous get/read/match accessor(s)? I'm thinking that conceptually the operations web apps usually need to perform are likely:

  1. what is the serialization of currently in-scope cookies?
  2. are any cookies with name X present?
  3. what are all the in-scope cookie values with name X?
  4. what is the value of the first-listed in-scope cookie with name X?
  5. what are the names of all in-scope cookies with names like Y?
  6. what are the names and values of all in-scope cookies?
  7. a specific list of operations of type 1, 2, 3, and/or 4, to be performed at a single instantaneous snapshot of the serialized in-scope cookies

For all "name X" cases, wildcard naming support (or at least prefix-matching support) is very helpful. This is useful when deciding which cookies to clear at logout and also for log-structured (or otherwise sharded) cookie operations designed to work reliably in the presence of slow network + inter-window race conditions.

@inexorabletash
Copy link
Member

Re: transactional behavior - I can think of a handful of different theoretical scenarios:

  • atomically get/set multiple cookies
  • perform operations (e.g. get, increment, set) bounded by a lock/mutex

And orthogonal constraints:

  • cooperative vs. competitive - e.g. an external mutex API would suffice, where atomicity is "opt in" vs. needing to bake this in to this spec
  • implicit vs. explicit - e.g. like IDB's transactions or the "storage mutex" where the start of operations begins an implicit transaction, vs. needing to invoke a transaction/operation scope and (maybe) explicitly commit/apply the changes.

There are probably more possibilities - I don't have good theoretical grounding here. :P But we should match up the possible space here vs. the actual use cases @bsittler enumerates later. If we can satisfy the use cases without needing to describe transactions at all in the spec and just provide set-multiple and get-multiple primitives, I think it'll be much, much easier for implementers and users. If we do need transactions then things get scary fast (e.g. are fetches blocked?)

@bsittler
Copy link
Contributor Author

Given the further discussion here and on the explainer (previously in now-merged #16 and addressing a few residual issues in #17) I think I will abandon this pull request - its original intent is certainly obsolete now! - and start over in a separate one.

@bsittler bsittler closed this Jul 27, 2016
@bsittler bsittler deleted the bsittler-patch-1 branch July 27, 2016 05:34
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

Successfully merging this pull request may close these issues.

3 participants