Adding Cookies.all() [Alias Cookies()] and supporting documentation

ScottHamper / Cookies

JavaScript Client-Side Cookie Manipulation Library

The Unlicense

1.77k stars 169 forks source link

Adding Cookies.all() [Alias Cookies()] and supporting documentation #12

Closed fresheneesz closed 11 years ago

fresheneesz commented 11 years ago

Seemed this logical last piece was missing

ScottHamper commented 11 years ago

Hey @fresheneesz,

Thanks for taking an interest in Cookies.js! Before I consider merging your pull request, would you mind describing a real world use case where this functionality would be ideal? I'm certainly open to adding new features, but I want to make sure I'm not adding more features just for the sake of adding more features. I'd like to keep the library as lean as possible.

fresheneesz commented 11 years ago

I mean, there are countless uses. Here are some uses:

in test (if you want to easily grab a list of cookies that you've set), to visually or programmatically verify cookies are set properly.
clearing all cookies, would be annoying to specify each and every one.
finding cookies with weirdly generated keys that fit a certain pattern (sometimes you're not the one that designed the site : / )

ScottHamper commented 11 years ago

Thanks @fresheneesz,

You've made some interesting suggestions and I'd like to address them.

For verifying that cookies are set properly, users are better off using dev tools within browsers, as more information about cookies is displayed. What I mean is, once a cookie is set via JavaScript, reading the cookie back from document.cookie only returns the key/value pair. All extra information about the cookie, such as the domain, path, whether it is secure or not, etc cannot be recovered. However, all of this information is displayed in browser tools. As such, Cookies.js is not intended to be a debugging tool.

Clearing all cookies would not be possible for similar reasons. You could iterate through all the keys that have values set, but a cookie cannot be cleared unless the path and domain properties match the original cookie. This is because a cookie is cleared by setting itself again, but with an expiration date in the past.

Lastly, I would argue your third case is also better handled by browser dev tools. However, this also seems like the most hypothetical use case suggested and not applicable to a real world scenario. If you can explain in better detail a situation where you've needed to do this, I would entertain the idea further.

The only advantage I can think of, in a broad sense. by providing the all() functionality, is that users could easily iterate through all cookie keys. I'm just having trouble thinking of real scenarios where that would be useful, given the intent of the library.

I hope none of my response has come across offensively - I really appreciate your willingness to contribute! I am definitely up for discussing this matter further.

fresheneesz commented 11 years ago

So in all these cases, I'm of course talking about doing this for a site the given page is from. E.g. I'm not talking about clearing your browser's cookies - I'm talking about clearing a user's cookies for your site.

I think clearing cookies is the best case I can think of, but beyond this, I have to question the push back on such a simple feature. I've seen this from a few github projects, where library creators want to keep their library "lean". However, instead of rejecting any feature that you can't think of enough concrete use-cases for, I would recommend focusing on constraining the conceptual scope of your library, since that will lead you toward creating a better library.

Tools and libraries should allow the users of those tools to do things the creators of those tools never even thought of. How can you do that if you require that every feature have multiple concrete use cases?

A better argument than use cases is that allowing access to the list of cookies is conceptually simple, structurally simple, and completes the functionality that cookies provides - a list of expirable entries that can conditionally apply to a given url. Your library simply isn't complete without this piece. Without it, a person would have to re-implement almost half of your library just to do it.

ScottHamper commented 11 years ago

I agree that it could be neat to be able to clear all of a user's cookies for your web app, but it is unfortunately not possible to reliably do due to how the native JS API is implemented. This is better done by the user deleting cookies through their browser, or by deleting cookies on the server side. The only reliable way to delete all cookies using Cookies.js is through manually deleting each cookie, as some cookies may not use default paths/domains, and only the developer can be aware of these additional details.

As far as API design goes, I want to say that I am not reluctant to add this feature because there are not enough use cases, but because I cannot think of any practical use cases where the all() functionality would be beneficial (that's not to say they don't exist, however). The cases you've suggested so far have not convinced me that this feature would provide value (and indeed, 2 out of the 3 use cases cannot be handled well by Cookies.js due to how the native API is implemented). The feature is definitely simple, but it still adds complexity that I would rather avoid unless there is a reasonable justification for including it.

To answer your question of how to accommodate users that think of ways to use the library that I had not originally thought of - that's what this GitHub repo is for! I welcome users to communicate with me about the shortcomings they run into with Cookies.js when working on projects. At that point it can be discussed whether or not the library can overcome the shortcoming, or if in like the case of deleting all cookies, why it cannot.

fresheneesz commented 11 years ago

Ok fair enough, there are limitations on what cookies can be accessed. So you're saying that while you can update (or delete) a cookie via Cookies("someCookie", someValue), you won't necessarily see it when you try Cookies("someCookie") (or using the Cookies() functionality I added)? I take it the edge case is if the cookies you're looking for have a path of a different page than the user is currently on, correct?

I'm still a little confused though - it doesn't make sense to me that you could alter the cookie for another path, and at the same time, not be able to get its value. But maybe thats exactly how the native cookie api works?

ScottHamper commented 11 years ago

I think an example will best demonstrate what I mean:

Cookies.set('key', 'value'); // Cookie now set with default path of '/' and default domain
Cookies.set('key', 'fresheneesz', { path: '/more/specific/path' }); // Second cookie created with a more specific path

// While browsing '/more/specific/path':
Cookies.get('key'); // "fresheneesz"

Cookies.expire('key'); // Removes the cookie with default path of '/', but the cookie with a more specific path still exists

Cookies.get('key'); // Still "fresheneesz"

Cookies.expire('key', { path: '/more/specific/path' }); // Now the second cookie has been removed as well

What expires() is basically doing in vanilla JS is:

document.cookie = 'key=;path=/more/specific/path;expires=Thu, 11 Jul 2013 22:30:13 UTC';

After setting both cookies in the first example, document.cookie would look like this when browsing '/more/specific/path':

document.cookie; // "key=fresheneesz;key=value"

Basically a cookie is defined by the combination of its key, path, and domain, but when reading a cookie out from the document.cookie string, only the key and value are provided. It is possible to have duplicate keys, but the most specific cookie to the current domain/path will be retrieved by Cookies.js and server side frameworks.

This is why deleting all cookies inside of a loop is unreliable - if the user of the library never changes the default path/domain, there won't be an issue, otherwise there is no way for the library to retrieve the non-default path/domain that has been set.

fresheneesz commented 11 years ago

Yeah Ok, that does make it much clearer. You know what would be cool, tho totally not in line with how you've set up your API, is if you had keys set up as paths like:

NewCookies("/key", "someValue"); // Equivalent: Cookies("/key", "someValue");
NewCookies("/specific/path/key", "someValue"); // Cookies("/specific/path/key", "someValue", {path: "/specific/path"});
NewCookies("key", "someValue"); // Equivalent: NewCookies("/key", "someValue");

Upside is that if you use this api, you could always find a full list of cookies. Downside is that you wouldn't have the full range of Cookiage without some additional "more raw" functions. Then again, I probably wouldn't design something to use page-specific cookies, so it wouldn't really help me ; )

ScottHamper commented 11 years ago

That's a pretty interesting idea for a work around. You would just need to extend it a little further for domains as well.

Thanks again for taking time to collaborate with me - if you come up with any more ideas for the library in the future, I'd love to hear them! I'll feel bad if you spend effort on modifications that might not make it into the official master branch, though, so I would start a discussion before diving in! Unless you just wanna go for it - then all the power to you.