search

ScottHamper / Cookies

JavaScript Client-Side Cookie Manipulation Library
The Unlicense
1.77k stars 169 forks source link

Is JSON-encoding by default really a good idea? #4

Closed inadarei closed 12 years ago

inadarei commented 12 years ago

Very nice library. Having split feelings about its by-default JSON encoding of the cookie value, however. This causes simple string literal cookie values to be saved with wrapped double-quotes around 'em. If you use this library for both reading and saving values - not such a big problem. The thing about cookies - they are usually accessed by a server-side technology too and that's where things get weird. You need to run an additional json-decode on the server-side to remove "extra" double quotes.

It would be nice if there were a way to disable by-default JSON encoding through options.

ScottHamper commented 12 years ago

I think requiring a JSON decode server-side is reasonable.

For string values, it's overkill to JSON encode, but for any other data type it's going to be necessary (at least if you want to retrieve a value in the same data type it was put in). I thought about modifying the library to not encode string values, as the library would still access the value properly. However, this means that if a user always runs cookie values through a decode step on the server, it will throw an exception for string values because they won't be properly encoded.

I'd rather keep the API consistent than introduce an edge case just to provide a shortcut. Ideally, there would be a function server-side for setting cookie values that automatically does its own JSON encoding, and a function server-side for getting cookie values that automatically does it own JSON decoding. These two functions would be re-used anywhere cookies are interacted with, and everything just works.

inadarei commented 12 years ago

Scott, if you don't mind my objection: I will have to respectfully disagree regarding which one is the "edge case" :) Most cookies on the web are literal key/value pairs. Encoding structured data in JSON for cookies, is an interesting use-case, but imho not the intended usage by RFC 2109.

I think the API will be cleaner/simpler if it does not take on "extra" responsibility of encoding JSON objects and just deals with elegantly hiding the ugly details of dealing with cookies in Javascript. If somebody wants to add JSON encoding on top of basic functionality, it's pretty straightforward in JS.

I think this is the case where removal of extraneous feature is more troublesome than adding the feature if it is needed.

Thanks

ScottHamper commented 12 years ago

Yea, I see what you're saying. I was conflicted because I considered the JSON encoding/decoding to be a core feature of the library, but when you think about it, it probably shouldn't be.

I'll update the repo tonight and remove the auto JSON stuff.

inadarei commented 12 years ago

Thanks. And many thanks for writing the library in the first place. Very nice. Much appreciated.

ScottHamper commented 12 years ago

Glad you like it!

I'm going to re-open this issue, though, so I can close it tonight from a commit. Keeps me organized!

inadarei commented 12 years ago

Oops. Sorry :)

ScottHamper commented 12 years ago

Sorry - schedule ended up being busier than expected. Might be a couple days. I've already changed the library, I just need some time to update tests and documentation.