Allow Objects and Arrays to be stored as JSON

[dhoulb]

Continued from #283

Messed up the previous pull request, so I've made a fresh clean one. This one also removes the json configuration directive, in favour of the automatic JSON object and array parsing.

README updated to reflect the new behaviour. Tests updated, and additional tests added to test new functionality.

[FagnerMartinsBrack]

I think removing json config now will break a lot of systems. The implemention could be done without removing the feature (even if that is deprecated).

[carhartl]

I liked the idea of releasing a minor version with all the latest changes, and then a 1.5 beta including this change.

Just thinking, isn't removing json config doing any harm at all? Anyone not using it will get automatic json parsing for free, anyone having it turned on will still get JSON parsed out, no?

[carhartl]

Unless I'm overlooking something the only thing that isn't backwards compatible here is when someone is storing values other than Array or Object with json config active, like booleans.

See my comment here: #286 (comment)

[carhartl]

Another thing I'm thinking about is whether it's worth it to move all of the parsing logic out of the plugin, and instead utilize data(), thus have a cookie read behave exactly like it. Either by using a dummy node, or passing an object to data() that quacks like a node.

[dhoulb]

Whether json config is turned on or not should make no real difference.

The one thing I've noted is: if json config is on but the JSON cookie cannot be parsed, it used to return undefined — it'll now return the unmodified string. Done this way so that JSON-like false positives that get matched by the RegEx, such as [hello], will work fine.

[dhoulb]

@carhartl
Yeah, I like the idea of more integration, but it just seems a tad too hacky to create a phantom node etc? If they moved just the string-to-native logic from their dataAttr() function into its own function, that'd be perfect for us. I guess jQuery's open source — we could contribute the change and see if they take it.
https://github.com/jquery/jquery/blob/master/src/data.js

Now that I'm re-reading it, they do testing for === "true", ==="false", and number conversion that I haven't used — it'd be useful to have that too, so people always get the native type and not the string.

[carhartl]

Exactly. Imo reading from a cookie should behave exactly like jQuery.data(), not just in parts. If we get to avoid duplicated code that were just a welcome side-effect.

E.g. as if the following would work:

jQuery.data(document.cookie, 'fookey');

[FagnerMartinsBrack]

Yes, I was refering to some details of the json config that will not be backwards compatible such as the undefined return. I think that kind of change is made for a major release because it could bring some unexpected breaks.

I like the idea of exposing dataAttr from jQuery. But first we need to make sure there is no other way for doing this and if they would support such API to be exposed.
By the way, the idea is to expose only the relevant part of the API that makes the json conversion, that would require more work in jQuery's side.

But there is also the version compatibility issue, older versions of jQuery could not have jQuery.data behaving as it currently is, and if some change is done it would work only or newer versions.

[carhartl]

I am no longer sure if we should go for pushing a new jQuery API method to use, as this would make the plugin require the latest version, which seems needlessly restrictive?

[FagnerMartinsBrack]

Not only require latest version but depend on external support (I am pretty sure jQuery team will not create a new API for this reason :D)

Looks like the only feasible solution here is code duplication (as it is already being done).

[carhartl]

Not sure about the duplication. I'm trying a bit tonight to see whether we can use jQuery.data() feasibly.

[carhartl]

I really wouldn't want to duplicate the entire type parsing logic.

[dhoulb]

You'd only be using similar logic for reading cookies. It's not huge either, just a couple of lines — you already had most of it for the json configuration setting. All I've added is a RegEx test for [ or {, instead of the if (config.json) setting.

There is no logic for writing in .data(), as it never writes the string back. All I've changed in the writing code is replace if (config.json) with if (value.constructor === Object || value.constructor === Array. Again, nothing too crazy, but it lets your code outside of jquery-cookie be much cleaner.

I'd guess the reason jQuery don't expose it is because they figured it's such a tiny bit of logic, no-one would ever mind duplicating it. I think after you've removed the json setting logic, the difference for us in code size is pretty marginal. A couple-hundred bytes or so, compressed.

[carhartl]

Wasn't it also pulling true/false values back out? Maybe numbers?

To me this isn't even so much about the duplication. I see the benefits in reducing friction and future maintenance effort. If we use data(), reading a cookie value will always behave the same, per jQuery version that is. And if at some point the method is enhanced to do something new, we/users of the plugin get that for free, without us having to adapt anything at all.

All that being said, I'm not even sure whether it can work like that, to me that would just be the ideal I'd like to give a shot at.

[FagnerMartinsBrack]

In this case I think the ideia of using a detached DOM still stands (if it actually works)...

[dhoulb]

I can see the benefit of that, I just can't see many situations where you'd want jquery-cookie to convert types other than basic native types, true, false, numbers, arrays and basic objects. You're right about the numbers — I think it makes sense to include that too.

Their full code is this:

function dataAttr( elem, key, data ) {
    var name;

    // If nothing was found internally, try to fetch any
    // data from the HTML5 data-* attribute
    if ( data === undefined && elem.nodeType === 1 ) {
        name = "data-" + key.replace( rmultiDash, "-$1" ).toLowerCase();
        data = elem.getAttribute( name );

        if ( typeof data === "string" ) {
            try {
                data = data === "true" ? true :
                    data === "false" ? false :
                    data === "null" ? null :
                    // Only convert to a number if it doesn't change the string
                    +data + "" === data ? +data :
                    rbrace.test( data ) ? jQuery.parseJSON( data ) :
                    data;
            } catch( e ) {}

            // Make sure we set the data so it isn't changed later
            data_user.set( elem, key, data );
        } else {
            data = undefined;
        }
    }
    return data;
}

So to fake it, you'd need a nodeType = 1 and a getAttribute() function. Seems like a lot overhead, and it'd be quite hard for people to trace how it was working, whereas just re-using the following:

data = data === "true" ? true :
    data === "false" ? false :
    data === "null" ? null :
    +data + "" === data ? +data :
    rbrace.test( data ) ? jQuery.parseJSON( data ) :
    data;

Is pretty neat and self-contained. I don't really see that code looking much different unless Javascript itself changes significantly.

[carhartl]

I think I soon will be convinced :)

[FagnerMartinsBrack]

@carhartl I think you will, because data attribute is a specification (internal implementations doesn't matter and aren't suppose to change).

Just document saying that the process is similar to the spec and you are good to go :D

[carhartl]

Interestingly:

var something = {}
$.data(something, 'foo', 42)
$.data(something, 'foo') // => 42

[carhartl]

No, not interesting, because $.data(something, 'foo', '42'); doesn't work...

[carhartl]

Nonetheless, here's a proof-of-concept: 65a9d24

It required only a tiny change (two lines) without any duplication of code from anywhere at all.

Downside (although that'd be the same with the given PR): The plugin would require JSON.stringify, though that might no longer be a problem today.

To speed up things that $('<div>') would be created once and cached, so it'd be a three line change. Let me think about all this once again... :)

[dhoulb]

I guess that's pretty solid — it's just a bit weird :D

If you're caching the <div>, you'd have to change the attribute name every time. data() caches data- attributes once they've been read.

var $div = $('<div>');
$div.attr('data-cookie', '[1, 2]').data('cookie'); // [1, 2]
$div.attr('data-cookie', '[1, 2, 3]').data('cookie'); // STILL [1, 2]

I don't think there's too much overhead instantiating each time. People really aren't going to call thousands of cookies — the expensive part is actually attaching to the DOM.

On balance though, because of the weirdness of this, I'd probably still vote for just duplicating the logic. The size saving isn't worth the potential for this breakage if jQuery decide to change something in data(). I think the fact it's consistent is enough, it's not as though data() is going to be massively expanded any time soon.

[carhartl]

Planning to release a version 2.0 with this, along with #295.

[dhoulb]

Sounds great :D

[Krinkle]

These are not safe ways of identifying arrays or objects (neither works with foreign objects, and the Object case doesn't account for non-plain objects, which JSON.stringify does). I'd recommend using value !== null && typeof value === 'object' which covers plain objects, arrays and any other type of object.

[Krinkle]

On second thought, you may want to take the approach of data serialisation as is done for memcached/redis systems. Which is to use stringify() for all values that are not integer or boolean (even plain strings). This due to the inherent ambiguity with free-form strings once stored.

[FagnerMartinsBrack]

Hi @dhoulb, just so you know JSON handling is being developed in branch 2.0.

Here are the initial docs of the feature. This is still in development, so any suggestion is welcome before landing in master.

Closing this since it was fixed in commit js-cookie/js-cookie@a52398d