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

Add JSON automatic handling #10

Merged
merged 1 commit into from Apr 15, 2015
Merged

Conversation

FagnerMartinsBrack
Copy link
Member

No description provided.

FagnerMartinsBrack added a commit that referenced this pull request Apr 15, 2015
@FagnerMartinsBrack FagnerMartinsBrack merged commit a52398d into remove-json Apr 15, 2015
@FagnerMartinsBrack FagnerMartinsBrack deleted the auto-json-handling branch April 15, 2015 18:04

js-cookie provides automatic JSON storage for cookies.

When creating a cookie you can pass an Array or Object Literal instead of a string in the value. If you do so, js-cookie store the string representation of the object according to the `JSON.stringify` api (if available):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly technical, it doesn't need to be an Array/Object literal, does it? E.g.

Cookies.set("foo", new Array("foo"))

In that case I'd rather write something like "...you can pass an instance of Array or Object..."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we would be required to support it right? I don't see a clear reason for using an Array instance, if we were to support that then in that case we should for consistency also support Object instance, but Object can be anything, it can be a Date instance, a String instance, a custom object with methods without any relationship with key:value pairs, etc.

We could document that an "Array instance" is allowed, in this case it would work for Array constructors and literals, but then would we also support Objects for consistency? Do you think this is bad because ppl might read "Literals" as objects that should not be stored in variables? The intent here is just to limit the creation of the instances using literal notations, which AFAIK already handles all use-cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could document that if it's not a valid JSON structure then it will use String(object) in the input, but then we would be enforcing behavior for invalid input, and any changes that does something else would be backwards incompatible.

Assuming we are using semver spec, backwards incompatible changes should only be release in major version (in this case v3.0).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but Object can be anything, it can be a Date instance, a String instance, a custom object with methods without any relationship with key:value pairs, etc.

Now I get your point. I was specifically remembering that once I needed to fix an issue so this works:

$.cookie("foo", new String("..."))

Than I thought, if we say "literals" we might be unnecessarily restricting users to use literals. I didn't think of all the other objects. I just thought new Array and new Object will probably work just as well already out of the box...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

When creating a cookie you can pass an array or plain object instead of a string

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's use that!

@carhartl
Copy link
Member

@FagnerMartinsBrack

I am really not happy about getJSON(), was it necessary? I thought the plan was to handle that automatically, within the same reader method...?

Unfortunately you didn't give me enough time to review beforehand.

@carhartl
Copy link
Member

You're too fast for me :)

@FagnerMartinsBrack FagnerMartinsBrack mentioned this pull request Apr 19, 2015
13 tasks
@FagnerMartinsBrack FagnerMartinsBrack added this to the 2.0 milestone Apr 20, 2015
@FagnerMartinsBrack
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants