Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Converters for writing/reading values #151

Closed
carhartl opened this Issue · 21 comments

3 participants

@carhartl
Owner

Allow setting up converters, which will give developers much gained flexibility over json = true.

// write and read converters
$.cookie.converter = [JSON.stringify, JSON.parse];
// single function becomes read converter
$.cookie.converter = function (value) { return parseInt(value); };

That could even make deprecation of json = true work (upon first read/write, JSON converters were set up when we find config.json to be true). We could also keep this config as a convenience shortcut for setting up the JSON converters.

Also: such a converter function could be build in a way that they skip values with say a particular prefix. That will make developers life much easier when dealing with JSON as well as standard cookies, in that they, when implementing a converter according to their needs, won't have to toggle (and remember when to toggle) the json config.

@carhartl
Owner

Converters would to need to have the cookie name passed as well to allow skipping conversion:

$.cookie.converter = function (value, name) { return name.indexOf('count') === 0 ? parseInt(value) : value; };

$.cookie('count_foo', 8);
$.cookie('count_foo'); // => 8
$.cookie('simple', '1');
$.cookie('simple'); // => '1'
@FagnerMartinsBrack
Collaborator

I personaly think passing an array is quite ugly.

$.cookie.converter = function( read, write ) {
    read(function( name, value ) {
        return value;
    });

    write(function( name, value ) {
        return value;
    });
};

It is probably possible to achieve something like this right?
What are the drawbacks?
Does it encourages user to call read/write several times?
Does it affect performance or character count (on the developer side)?
What you think?

@carhartl
Owner

@FagnerMartinsBrack Yes, array is ugly and wouldn't work well anyway:

// Is this a read or write converter?
$.cookie.converter = [function(value) { /* ... */ }];

// So I'd be forced to do this:
$.cookie.converter = [null, function(value) { /* read converter... */ }];

There's no such thing as you supposed, but I think the following should work well:

$.cookie.converter = {
    read: function(value) { /* ... */ },
    write: function(value) { /* ... */ } 
};

$.cookie.converter = {
    read: function(value) { /* ... */ }
};
@FagnerMartinsBrack
Collaborator

Yeah, what you proposed is the way to go for sure.

@carhartl carhartl was assigned
@carhartl
Owner

We could also define an API like this:

// write
$.cookie('foo', 'bar', function(value) { /* convert write... */ });
$.cookie('foo', 'bar', JSON.stringify);

// read
$.cookie('foo', function(value) { /* convert read... */ });
$.cookie('foo', JSON.parse);
$.cookie('foo', Number);

Seems more straight-forward, more flexible. We could avoid yet another configuration object such as $.cookie.converters. If the need arises, such thing could be build on top of the simple function passing.

@carhartl
Owner

@FagnerMartinsBrack ^ Any thoughts on this?

@FagnerMartinsBrack
Collaborator

Can't see better way to do this.

This way we already have the cookie name bound if a control is needed based on the key. The problem by adding a configuration object is the lack of context for a single cookie.
The Number constructor is a very good thought by the way.

It would be then:

$.cookie( 'foo', 'bar' );
$.cookie( 'foo', 'bar', {} );
$.cookie( 'foo', 'bar', function(value){} );
$.cookie( 'foo', 'bar', {}, function(value){} );

$.cookie( 'foo', function(value){} );

It matches every requirement and more:

  • You can convert when reading.
  • You can convert when writing.
  • You can pass native converter references that accept first argument as the value for both ways.
  • You can pass the same custom reference for both ways (not requiring global converters).
  • You can bind to a single cookie.
  • You can save characters.
@carhartl
Owner

Cool, thanks! I have it implemented already and will be pushing to a branch...

@carhartl carhartl referenced this issue from a commit
@carhartl Implemented converters as optional last function argument. Addresses #…
…151.

Also fixed along with it that certain decoding and sanitizing should not
happen for cookie keys.

Missing still:
- Add to changelog
- Documentation
d60b233
@carhartl carhartl referenced this issue from a commit
@carhartl Implemented conversion functions as optional last argument for readin…
…g and writing. Closes #151.

Along with it fixed that certain decoding and sanitizing should not happen for a cookie's key.
669535f
@carhartl
Owner

After implementing it this way I was getting some doubt about how useful it really is, at least for the JSON example:

Isn't this:

$.cookie('foo', { bar: 'baz' }, JSON.stringify);
$.cookie('foo', JSON.parse);

a more complicated way (in terms of expressiveness) of writing this:

$.cookie('foo', JSON.stringify({ bar: 'baz' }));
JSON.parse($.cookie('foo'));

Do we gain anything here?

In terms of numbers, I have a little less doubt:

$.cookie('foo', Number);

Seems to read a little better than:

Number($.cookie('foo'));

but maybe not either.

After all, it feels like a useless feature to me. All we wanted in the first place was a more elegant and flexible way to handle JSON (ideally automatically).

@carhartl
Owner

Also, aren't we there already yet? With #195 being fixed, one can set json = true and still save strings and json objects at the same time without having to worry about the flag. The flag now acts more of a manual verification by way of the developer that JSON.parse and JSON.stringify are available.

We could still keep the read conversion, though nobody ever has asked for this feature.

@carhartl
Owner

Ah no, with json = true we can't yet read "simple" cookies, cookies that haven't been stringified using JSON.stringify before saving that is. The plugin returns undefined in such a case.

How useful is it to return undefined in case attempting to parse a JSON cookie fails?

@carhartl
Owner

Ok, there was a long discussion in #132 what should be returned in case of invalid JSON. So we can't just return the string, whatever it is.

@carhartl
Owner

Conclusion:

Write conversion is obsolete.

Read conversion is useful since we enable developers to handle JSON and simple cookies at the same time while avoiding json = true. Using just JSON.parse was an example a bit too simple.

function converter(value) {
  try {
    return JSON.parse(value);
  } catch(e) {
    return value;
  }
}

$.cookie('simple', 'foo');
$.cookie('json', JSON.stringify({ foo: 'bar' }));

$.cookie('simple', converter); // => 'foo';
$.cookie('json', converter); // => {foo:"bar"}
@carhartl
Owner

That example only makes sense of course if we were setting up such converter as default somehow, for the simple cookie we would not have to pass it as an argument in the first place.

Ugh. Taking a break.

@FagnerMartinsBrack
Collaborator

Poluting the signature is something dangerous indeed, but what I think is the possibility of defining the converter without the need of wrapping the cookie.

function converterJSON( value ) {
    // Convert the JSON value
}

// Use custom converter
$.cookie(  'cookieObject', {
    home: 'google.com'
}, { path: '/' }, converterJSON );
$.cookie(  'cookieObject', converterJSON ); // { "home": "google.com" }
$.removeCookie( 'cookieObject' );

// I don't care about converter here, nothing changes
$.cookie( 'simple', 'value', { path: '/' } );
$.cookie( 'simple' ); // 'value'
$.removeCookie( 'simple', { path: '/' } );

I may have my custom way to store and read my cookies (perhaps I want to reduce characters in my requests)

// Default
$.cookie.converters = {
    read: function( value ) {
        // value (string) -> example:value,example2:value2
        // convert to object literal
        // { example: 'value', example2: 'value2' }
    },
    write: function( value ) {
        // value (object literal) -> { example: 'value', example2: 'value2' }
        // convert to string
        // 'example:value,example2:value2'
    }
}

// Custom converter
function converterJSON( value ) {
    // Convert regular JSON
}

// Use default
$.cookie( 'preferences', {
    example: 'value',
    example2: 'value2'
}, { path: '/' });
$.cookie( 'preferences' ).example2; // 'value2'
$.removeCookie( 'preferences', { path: '/' } );

// Use custom converter
$.cookie(  'cookieObject', {
    home: 'google.com'
}, { path: '/' }, converterJSON );
$.cookie(  'cookieObject', converterJSON ); // { "home": "google.com" }
$.removeCookie( 'cookieObject' );

What if I want to set up a single converter and take care of everything in one place?

$.cookie.converters = {
    // Need "name" as second argument or $.cookie( 'my_cookie', Number ) does not work.
    // Quite ugly since the convention is ( name, value )
    read: function( value, name ) {
        if ( name.indexOf( 'pref_' ) === 0 ) {
            // convert preferences
            // return preferences object literal
        } else if ( name.indexOf( 'json_' ) === 0 ) {
            // convert JSON
            // return json object literal
        }
    },
    write: function( value ) {
        if ( name.indexOf( 'pref_' ) === 0 ) {
            // convert preferences
            // return preferences string to be stored
        } else if ( name.indexOf( 'json_' ) === 0 ) {
            // convert JSON
            // return stringified JSON to be stored
        }
    }
}

$.cookie( 'pref_home', {
    url: 'github.com'
}, { path: '/' });
$.cookie( 'pref_home' ).url; // 'github.com'

This is what I can remember from previous discussions. Am I missing something?

After all, it feels like a useless feature to me. All we wanted in the first place was a more elegant and flexible way to handle JSON (ideally automatically).

Also the possibility to customize the way the conversion is handled

Write conversion is obsolete.

If we create a use case for the last example it is not.

Read conversion is useful since we enable developers to handle JSON and simple cookies at the same time while avoiding json = true. Using just JSON.parse was an example a bit too simple.

If you avoid $.cookie.json = true you cannot write JSON cookies but rely on server-side only.

The idea was to deprecate $.cookie.json = true. Like you are saying we keep the flag but create a converter? So we have a flag that say "all cookies are read and written as JSON" and a converter that say "Not quite, you can choose the way you can read in order to read simple cookies too"?

I don't get it, am I missing something?

@carhartl
Owner

Lots of confusion.

What I was saying is that converters only as function argument might not be all that useful - but this was my idea inbetween -, because there is not much difference in the resulting written code: $.cookie('foo', { bar: 'baz' }, JSON.stringify) vs $.cookie('foo', JSON.stringify({ bar: 'baz' })), the latter code even expresses the intent without some sort of obfuscation.

Thus function argument only converters wouldn't help much in being able to deprecate json = true - we would just have to tell developers to use JSON.stringify and JSON.parse themselves.

Thus I was thinking to return to setting up converters like $.cookie.converters = ... (While we can still allow converters being passed as an argument for quick, one time conversions).

Nevertheless, this feature has never been asked for and the reason to implement it was to make json = true more flexible (usable), thus I am reluctant to add it - if we could find a better (simpler) solution. Once it's there it we can not easily remove it again.

@FagnerMartinsBrack
Collaborator

I agree there is lots of confusion going on. This issue is opened for a long time and I am not currently working heavly with jquery cookie in a client side basis, which make it very dificult to get context about proper changes in the API.

The thing I had in mind at the moment you mentioned converters is what I mentioned earlier. So I was probably overthinking the problem.

Requirements

Lets make the requirements clear then, the API should meet all the aspects below:

  • The user should be able to handle JSON globally for all cookies
    • Should not mess with simple read/write.
    • Should not have to declare the API every time before a read/write.
  • The user should be able to handle JSON in a per cookie basis.
    • Should be able to write using object literals in the value.
    • Should be able to read JSON cookies and stringify

As far as I know there is no other requirements, if that is not the case just let me know.

An architetural problem?

Seems like in the past we had an architetural problem regarding the signature $.cookie( [String], [Object] ), mentioned at #93 (comment)
I does not seems to be a problem anymore right? Since a valid value is always used for writing https://github.com/carhartl/jquery-cookie/blob/master/jquery.cookie.js#L43
There is not even a test for that anymore oO

Proposals

So if that is the case and we have no dependency when writing, then we could determine a json cookie without any further configuration! Just check for a plain object literal in the second argument and we are good.
The bad thing is that $.cookie( 'my_cookie', { raw: true }) is not coming back.
Can you confirm if that is feasible?
If that is, we have just killed the writing problem.

We are able to identify JSON cookie when writing, thats fine. But we can't identify when reading because we have the server-side factor (so a prefix created by the plugin is not possible).
Here an option kicks in, but there is no need to be a converter:

$.cookie( 'my_json' ); //{ object: "literal" }

$.cookie.raw = true;
$.cookie( 'my_json' ); //"{ "not object": "literal" }"

That could break if you are relying in the JSON string for a different matter though.

Or just forget it all

If that is too much and the API locks are too frightening just let it be.
The developer can easly creating their own functions or monkey patch $.cookie to provide the proper conversions:

var old = $.cookie;
$.cookie = function() {
    // good luck
    return old.apply( null, arguments );
}

Deprecate $.cookie.json in favor of letting the developer take care of the conversions.
Let $.cookie.raw for those implementing a custom way to handle encoded cookies and that is it.
No need to complicate a lightweight plugin like this.

What is your opinion about this?
Am I missing something?

@carhartl carhartl referenced this issue from a commit
@carhartl Implemented conversion functions as optional last argument for readin…
…g and writing. Closes #151.

Along with it fixed that certain decoding and sanitizing should not happen for a cookie's key.
47be6ea
@klieber

Hi,

Just reading through this and wanted to make a suggestion. Have you considered something like this:

// create a new cookie instance with a converter
var $jsonCookie = $.cookie.withConverter({
  read: function(value,name) {
    return JSON.parse(value);
  },
  write: function(value,name) {
    return JSON.stringify(value);
  }
});

// normal behavior
$.cookie('foo', 'bar'); // write string 'bar'
$.cookie('foo'); // return 'bar'

// using converter
$jsonCookie('biz', {example: 'bang'}); // converts object to json string then writes
$jsonCookie('biz'); // reads json string and converts back to object

You could even get a little more creative and do it like this:

$.cookie.addConverter('json', {
  read: function(value,name) {
    return JSON.parse(value);
  },
  write: function(value,name) {
    return JSON.stringify(value);
  }
});

// normal behavior
$.cookie('foo', 'bar'); // write string 'bar'
$.cookie('foo'); // return 'bar'

// using converter
$.cookie.json('biz', {example: 'bang'}); // converts object to json string then writes
$.cookie.json('biz'); // reads json string and converts back to object

That would lead to the possibility of still adding a converter to the main cookie function if needed:

// add a converter to the main function
$.cookie.addConverter({
  read: function(value,name) {
    return value.replace('z','a');
  },
  write: function(value,name) {
    return value.replace('a','z');
  }
});

// add a special json converter
$.cookie.addConverter('json', {
  read: function(value,name) {
    return JSON.parse(value);
  },
  write: function(value,name) {
    return JSON.stringify(value);
  }
});

// using main function with converter
$.cookie('foo', 'bar'); // write string 'bzr'
$.cookie('foo'); // return 'bar'

// using special json converter
$.cookie.json('biz', {example: 'bang'}); // converts object to json string then writes
$.cookie.json('biz'); // reads json string and converts back to object

Take it or leave it.

@carhartl
Owner

Thanks @klieber, I think this is interesting. Since it would even allow to add more than one converter without having to clobber everything into one function.

@FagnerMartinsBrack
Collaborator

I liked the last examples, the problem is the compatibility with the flags $.cookie.raw and $.cookie.json.

Remove them and provide default functions in the place may be the best approach and may be possible without breaking backwards compatibility if we play with ECMA 5 get/set spec, but that would drastically reduce the browser compatibility of the plugin (bah, a fast research told me only IE9 +)

Or prevent them to be registered...

@FagnerMartinsBrack
Collaborator

Hmm or not, the same page has different examples, need to take a look in that.

@carhartl carhartl referenced this issue from a commit
@carhartl Implemented conversion function as optional last argument for reading…
…. Also see #151.

Along with it fixed that certain decoding and sanitizing should not happen for a cookie's key.
64eb7b7
@carhartl carhartl closed this
@carhartl carhartl referenced this issue from a commit
@carhartl Implemented conversion functions as optional last argument for readin…
…g and writing. Closes #151.

Along with it fixed that certain decoding and sanitizing should not happen for a cookie's key.
e879ffb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.