Is it time in v3 to dump getJSON either in favor of people doing JSON.stringify(Cookies.get('name')), Cookies.get('name', JSON.parse(value))?
I don't see a valid point in encouraging things like Cookies.getJSON().a.foo.bar -> If one level of JSON is removed from the store, the code crashes.
@carhartl While researching for past issues on this I found https://github.com/js-cookie/js-cookie/issues/416#issuecomment-374518807. You might be interested in your own comment about a potential v3.
FagnerMartinsBrack
All 18 comments
If there are no comments on this what shall we do? Assume to dump it? Assume to keep it?
FagnerMartinsBrack
on 2 Oct 2019
I don't want this to hold v3 release
FagnerMartinsBrack
on 2 Oct 2019
In doubt keep it, though imo it really isn’t needed (because JSON.stringify() etc.)
An idea I had was to run a query on the GitHub dataset and see how often it would be used.
carhartl
on 2 Oct 2019
I was planning to do a beta release anyway first:
npm run release major -- --preRelease=beta --dry-run
so we'd still be able to remove things.
carhartl
on 2 Oct 2019
Not everyone is going to need this. Throwing out ideas: we could provide this as an additional package, with js-cookie as a dependency. Or, now that we‘ve started with modules, we could try to further modularize, and rely on treeshaking for removing unused code (this would likely also apply to get and set)..
carhartl
on 3 Oct 2019
One of the features of js-cookie is the no reliance on third-party tools, I believe relying on Tree Shaking to keep the lib small could be a mistake. I don't use Tree Shaking in most of my projects, for example, I simply design the page states in a way I don't introduce unnecessary JavaScript code.
Having another package seems to be a good option. However, I would suggest to not do it until someone comes along complaining about the functionality not being there anymore.
Honestly, JSON.stringify(Cookies.get(...)) and Cookies.set(JSON.parse(...)) seemed to be a userland concern since the day one. Better to use composition over baking in functionality to map data around (unless it's data that encapsulates complexity unique to cookie handling, like the default Cookies.withConverter() behavior)
FagnerMartinsBrack
on 4 Oct 2019
One of the features of js-cookie is the no reliance on third-party tools, I believe relying on Tree Shaking to keep the lib small could be a mistake
Yeah, I wasn't clear about this. What I meant was that _should we decide to keep this feature_ we could still give people the means to come up with a smaller bundle, by for instance enabling treeshaking - might still be worth keeping an eye on going forward with modules.
Having another package seems to be a good option. However, I would suggest to not do it until someone comes along complaining about the functionality not being there anymore.
👍
Honestly, JSON.stringify(Cookies.get(...)) and Cookies.set(JSON.parse(...)) seemed to be a userland concern since the day one.
I think the same. (The only tiny advantage I can still see we're providing is that a user could pass an object without worrying about stringifying it: Cookies.set('foo', { bar: 'baz' })
To sum this up - we're going to remove the built-in json support and point users to JSON.parse(Cookies.get(...)) and Cookies.set('foo', JSON.stringify(...)). Most likely in the FAQs 😁
I may get this done later today and will release a beta of v3, if there are no objections.
carhartl
on 4 Oct 2019
LGTM, we might have to update the size of code, hope we get much less than 900 bytes now ;)
FagnerMartinsBrack
on 4 Oct 2019
We're at 786 bytes now...
carhartl
on 5 Oct 2019
To sum this up - we're going to remove the built-in json support and point users to JSON.parse(Cookies.get(...)) and Cookies.set('foo', JSON.stringify(...)). Most likely in the FAQs 😁
Shouldn't this be a use case for converters ?
var JsonCookies = Cookies.withConverter({
write: JSON.stringify,
read: JSON.parse
});
nreynis
on 3 Dec 2019
@nreynis I tried that in the past. It turns out that you don't need the conversion function to flow through the lib, you would be just moving the code elsewhere, which doesn't provide much value:
JSON.parse(Cookies.get('jsoncookie'))
vs
var JsonCookies = Cookies.withConverter({
read: JSON.parse
});
JsonCookies.get('jsoncookie');
Plus the complication on how to deal with the return types: is it string or JS Object Literal on JsonCookies.get('jsoncookie');?
One thing that is a tough job to do is to get the escaping correctly so that it works in all browsers per spec. That's what the converters are for.
FagnerMartinsBrack
on 3 Dec 2019
you would be just moving the code elsewhere, which doesn't provide much value:
Well I'm a typescript user, so from my point of view there's a clear value here because you can use generics to infer typing on cookies you write/read.
Plus the complication on how to deal with the return types: is it string or JS Object Literal on JsonCookies.get('jsoncookie');?
This should return the same type than the read converter. (that's what the code does right now and it seems intuitive for me).
One thing that is a tough job to do is to get the escaping correctly so that it works in all browsers per spec. That's what the converters are for.
@FagnerMartinsBrack that's interesting, I would have though that the encoding would be pretty standardized and that once implemented there would be no need to provide an API to configure it.
If I understand the intent, converters should always get a string as input and output another string ?
I'm asking here because I'm working on a PR for the typing. The current one use a really convoluted generic system for converters which don't really help.
I've submitted a reworked definition here:
https://github.com/DefinitelyTyped/DefinitelyTyped/pull/40710
But if you're telling me this is an hard choice, then we're arguing on an implementation detail. If this is the case the real action here is to remove all the generics and type the converters to enforce string usage.
nreynis
on 3 Dec 2019
Well I'm a typescript user, so from my point of view there's a clear value here because you can use generics to infer typing on cookies you write/read.
You can still use the type system by wrapping your interface into js-cookie. That works well for whoever wants to use typescript.
I would have though that the encoding would be pretty standardized and that once implemented there would be no need to provide an API to configure it.
The encoding is pretty standardised. However, some servers historically fail to implement the spec correctly, see the SERVERS_SIDE.md file. I got in touch with the spec authors at the time and I couldn't find evidence they had looked at major server implementations when drafting the spec. That's why you need a way to override in case you're integrating with a faulty server-side implementation for reads from client side. That's why converters are there.
But if you're telling me this is an hard choice, then we're arguing on an implementation detail. If this is the case the real action here is to remove all the generics and type the converters to enforce string usage.
Just because js-cookie doesn't have one specific feature, that's not an evidence all the types that exist right now need to be removed.
FagnerMartinsBrack
on 3 Dec 2019
If I understand the intent, converters should always get a string as input and output another string ?
No, converters take cookie name and cookie value so that the decoding mechanism can operate on both properties, not just the cookie value. Maybe there are server side discrepancies on the cookie name also, I'm not sure.
FagnerMartinsBrack
on 3 Dec 2019
You can still use the type system by wrapping your interface into js-cookie. That works well for whoever wants to use typescript
Yeah sure you could but why would I? If my IDE tell me there's a 'withConverter' method that does it for me and cherry on the cake it even take a generic and propagate the typing everywhere. I would really be a fool to not use it!
Users won't dig through tickets to see this discussion. If the method isn't meant to do that this should be clearer.
nreynis
on 3 Dec 2019
If I understand the intent, converters should always get a string as input and output another string ?
No, converters take cookie name and cookie value so that the decoding/encoding mechanism can operate on both properties, not just the cookie value
Are you sure about that? Seems to me it act only on the value:
https://github.com/js-cookie/js-cookie/blob/ac337a9432182c02cc291b13798c0a1fbfa0e3a4/src/js.cookie.mjs#L38
Also there's a feature request opened for a key converter
nreynis
on 3 Dec 2019
I edited the comment before your responded
On Wed, 4 Dec 2019 at 09:18, Nicolas Reynis notifications@github.com
wrote:
If I understand the intent, converters should always get a string as input
and output another string ?No, converters take cookie name and cookie value so that the
decoding/encoding mechanism can operate on both properties, not just the
cookie valueAre you sure about that? Seems to me it act only on the value:
Also there's a feature request opened for a key converter
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/js-cookie/js-cookie/issues/540?email_source=notifications&email_token=AAGMCEL4KX6Y3J6GFCT5YY3QW3LJRA5CNFSM4IWWLR42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF3ALGI#issuecomment-561382809,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAGMCELLUDWBNCZ5X5NH23DQW3LJRANCNFSM4IWWLR4Q
.>
Fagner Brack
[image: https://]about.me/fagnerbrack
https://about.me/fagnerbrack?promo=email_sig&utm_source=email_sig&utm_medium=email_sig&utm_campaign=external_links
FagnerMartinsBrack
on 3 Dec 2019
The fact to can't decode/encode the cookie name is probably a mistake
On Wed, 4 Dec 2019 at 09:21, Fagner Martins eu@fagnermartins.com wrote:
I edited the comment before your responded
On Wed, 4 Dec 2019 at 09:18, Nicolas Reynis notifications@github.com
wrote:If I understand the intent, converters should always get a string as
input and output another string ?No, converters take cookie name and cookie value so that the
decoding/encoding mechanism can operate on both properties, not just the
cookie valueAre you sure about that? Seems to me it act only on the value:
Also there's a feature request opened for a key converter
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/js-cookie/js-cookie/issues/540?email_source=notifications&email_token=AAGMCEL4KX6Y3J6GFCT5YY3QW3LJRA5CNFSM4IWWLR42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF3ALGI#issuecomment-561382809,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAGMCELLUDWBNCZ5X5NH23DQW3LJRANCNFSM4IWWLR4Q
.>
Fagner Brack
[image: https://]about.me/fagnerbrack
Fagner Brack
[image: https://]about.me/fagnerbrack
https://about.me/fagnerbrack?promo=email_sig&utm_source=email_sig&utm_medium=email_sig&utm_campaign=external_links
FagnerMartinsBrack
on 3 Dec 2019
Related issues
DeadcatDev
·
3Comments
nanndemoiikara
·
5Comments
Daijobou
·
6Comments
yaconnn
·
6Comments
hrahimi270
·
6Comments
Most helpful comment
Shouldn't this be a use case for converters ?