Skip to content
This repository was archived by the owner on Nov 15, 2017. It is now read-only.

Handle batch object cookie read/write operations #123

Closed
johan wants to merge 4 commits into from
Closed

Handle batch object cookie read/write operations #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5f41fc5
Implemented $.cookie() => { key: val, ... } and $.cookie({ key: val, …
johan Oct 28, 2012
bcadea6
Fixed broken tests (assumed no cookies present).
johan Oct 28, 2012
31657d9
Tests for $.cookie(), $.cookie(obj) and $.cookie(obj, options)
johan Oct 28, 2012
72ee1e5
Docs.
johan Oct 28, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,26 @@ Read cookie:
$.cookie('the_cookie'); // => "the_value"
$.cookie('not_existing'); // => null

Delete cookie:
Read all cookies:

$.cookie(); // => { "the_cookie": "the_value", "...remaining": "cookies" }

Delete cookie (*):

// Returns true when cookie was found, false when no cookie was found...
$.removeCookie('the_cookie');

// Same path as when the cookie was written...
$.removeCookie('the_cookie', { path: '/' });

Edit multiple cookies in batch:

$.cookie({ set_me: 'to this', delete_me: null }); // => same object returned
// $.cookie('set_me'); // => "to this"
// $.cookie('delete_me'); // => null (*)

$.cookie({ go: '?', you: '!' }, { path: '/' }); // options object, as above

*Note: when deleting a cookie, you must pass the exact same path, domain and secure options that were used to set the cookie, unless you're relying on the default options that is.*

## Configuration
Expand Down
23 changes: 20 additions & 3 deletions jquery.cookie.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,14 @@

var config = $.cookie = function (key, value, options) {

if (typeof key === 'object') {
Copy link
Copy Markdown
Collaborator

@FagnerMartinsBrack FagnerMartinsBrack Dec 3, 2012

Choose a reason for hiding this comment

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

I think $.isPlainObject would fit better here...

Copy link
Copy Markdown
Author

@johan johan Dec 3, 2012

Choose a reason for hiding this comment

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

Unless there's a supported, intuitive use case for passing a null key, that just makes the code longer and slower.

Copy link
Copy Markdown
Collaborator

@FagnerMartinsBrack FagnerMartinsBrack Dec 3, 2012

Choose a reason for hiding this comment

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

I believe performance is not an issue here since the check is not in a loop.
And typeof a==='object' has one more char than $.isPlainObject(a) oO

for (var name in key) {
if (!key.hasOwnProperty(name)) continue;
$.cookie(name, key[name], value); // "options" is optional 2nd arg here
}
return key;
Copy link
Copy Markdown
Contributor

@Krinkle Krinkle Dec 2, 2012

Choose a reason for hiding this comment

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

Why return key here?

Copy link
Copy Markdown
Author

@johan johan Dec 3, 2012

Choose a reason for hiding this comment

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

If you followed the flow of the code, key here is the object of key: value pairs you just set, so you can use it just as any other assignment in javascript, e g: a = b = { x: 1, y: 2 }; sets both a and b to the given object. If you insert a $.cookie call anywhere in that assignment chain, this makes it still work, which is expected, intuitive and useful behaviour.

Copy link
Copy Markdown
Contributor

@Krinkle Krinkle Dec 3, 2012

Choose a reason for hiding this comment

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

I know what the value of key is, but I disagree about it being "expected and intuitive". Quite the contrary. More like unexpected, undocumented, inconsistent and encouraging bad coding patterns.

$.cookie('a', 'aaa', options); // returns undefined

$.cookie({a: 'aaa', b: 'bbb', options); // returns pairs object by reference

Aside from it making the return value inconsistent based on invocation signature, it is somewhat counter-intuitive. Why would it return the pairs object? I'd expect maybe a boolean return on whether the action was successful, or maybe the expiration date of the set cookies, or perhaps the newly formed cookie-syntax string. But not the pairs object.

All that just to save a literal assignment?

var values = $.cookie({a: 'aaa', b: 'bbb'}, options);
// Instead of:

var values = {a: 'aaa', b: 'bbb'};
$.cookie(values, options);

Copy link
Copy Markdown
Collaborator

@FagnerMartinsBrack FagnerMartinsBrack Dec 3, 2012

Choose a reason for hiding this comment

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

Since the issue here is the batch cookie operation I think this return change should be threated in another PR.
The ´$.cookie(name, value)´ returns the string representing the cookie and it is used in a few tests, I don't think that is a good idea either, the plugin should return undefined if there is no documented feature specifying what it should return...

Copy link
Copy Markdown
Author

@johan johan Dec 3, 2012

Choose a reason for hiding this comment

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

It is documented higher up in the pull request. You are all welcome to make your own pull requests based on this one changing as much as you like to whatever you personally deem more useful or intuitive. Personally, my opinion stands as is, and also, I don't plan to have computerized net access for most of the month of December anyway, so little will change here.

That said, I don't believe the repository maintainer has the time or interest in accepting pull requests; this and other good patches have been sitting untouched rather long.

(Yes. The whole pull request is to save myself and others writing this assignment loop and storing away the key:val pairs object outside of $.cookie.)

Copy link
Copy Markdown
Owner

@carhartl carhartl Dec 3, 2012

Choose a reason for hiding this comment

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

Interest yes, time hardly.

Copy link
Copy Markdown
Owner

@carhartl carhartl Dec 3, 2012

Choose a reason for hiding this comment

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

And regarding this PR in particular, I am still reluctant to add it, since I'd like to see whether there is greater popular demand, otherwise I'd like to keep the codebase as light as possible.

}

// write
if (value !== undefined) {
options = $.extend({}, config.defaults, options);
Expand All @@ -46,17 +54,26 @@
}

// read
var all = arguments.length ? null : {};
var decode = config.raw ? raw : decoded;
var cookies = document.cookie.split('; ');
for (var i = 0, l = cookies.length; i < l; i++) {
var parts = cookies[i].split('=');
if (decode(parts.shift()) === key) {
if ((name = decode(parts.shift())) === key || all) {
var cookie = decode(parts.join('='));
return config.json ? JSON.parse(cookie) : cookie;
if (config.json) {
cookie = JSON.parse(cookie);
}
if (all) {
all[name] = cookie;
}
else {
return cookie;
}
}
}

return null;
return all;
};

config.defaults = {};
Expand Down
48 changes: 42 additions & 6 deletions test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
var before = {
setup: function () {
cookies = document.cookie.split('; ')
var cookies = document.cookie.split('; ');
for (var i = 0, c; (c = (cookies)[i]) && (c = c.split('=')[0]); i++) {
document.cookie = c + '=; expires=' + new Date(0).toUTCString();
}
Expand Down Expand Up @@ -36,7 +36,7 @@ test('decode', 1, function () {
});

test('decode pluses to space for server side written cookie', 1, function () {
document.cookie = 'c=foo+bar'
document.cookie = 'c=foo+bar';
equal($.cookie('c'), 'foo bar', 'should convert pluses back to space');
});

Expand All @@ -45,6 +45,16 @@ test('[] used in name', 1, function () {
equal($.cookie('c[999]'), 'foo', 'should return value');
});

test('no arguments', 3, function () {
document.cookie = 'x=y';
var count = document.cookie.split(';').length;
var cookies = $.cookie();
equal(typeof cookies, 'object', 'should return object');
var found = 0; for (var key in cookies) found++;
equal(found, count, 'should find all cookies');
equal(cookies.x, 'y', 'should decode cookies');
});

test('raw: true', 2, function () {
$.cookie.raw = true;

Expand Down Expand Up @@ -81,7 +91,7 @@ asyncTest('malformed cookie value in IE (#88, #117)', 1, function() {
ok(true, 'N/A');
}
};
iframe.src = '/sandbox.html';
iframe.src = 'sandbox.html';
document.body.appendChild(iframe);
});

Expand All @@ -108,6 +118,27 @@ test('number', 1, function () {
equal($.cookie('c'), '1234', 'should write value');
});

test('editing cookies by object', 4, function() {
var edits = { a: 'a', c: null };
var result = $.cookie(edits);
ok(result === edits, 'should return the object instance passed');
deepEqual(result, { a: 'a', c: null }, 'should return the object unmodified');
equal($.cookie('a'), 'a', 'should set all given cookies');
equal($.cookie('c'), null, 'should remove nulled cookies');
});

test('editing cookies by object with options', 4, function() {
$.cookie('a', 'something');
$.cookie('c', 'something');
var edits = { a: null, c: 'c' };
var options = { expires: -1 };
var result = $.cookie(edits, options);
ok(result === edits, 'should return the object instance passed');
deepEqual(result, { a: null, c: 'c' }, 'should return the object unmodified');
equal($.cookie('a'), null, 'should use the options object for all cookies');
equal($.cookie('c'), null, 'should use the options object for all cookies');
});

test('expires option as days from now', 1, function() {
var sevenDaysFromNow = new Date();
sevenDaysFromNow.setDate(sevenDaysFromNow.getDate() + 7);
Expand Down Expand Up @@ -137,6 +168,8 @@ test('defaults', 2, function () {
$.cookie.defaults.path = '/';
ok($.cookie('c', 'v').match(/path=\//), 'should use options from defaults');
ok($.cookie('c', 'v', { path: '/foo' }).match(/path=\/foo/), 'options argument has precedence');
$.removeCookie('c', { path: '/foo' });
$.removeCookie('c', { path: '/' });
});

test('raw: true', 1, function () {
Expand All @@ -149,7 +182,8 @@ test('json: true', 1, function () {

if ('JSON' in window) {
$.cookie('c', { foo: 'bar' });
equal(document.cookie, 'c=' + encodeURIComponent(JSON.stringify({ foo: 'bar' })), 'should stringify JSON');
var first = document.cookie.split(';')[0];
equal(first, 'c=' + encodeURIComponent(JSON.stringify({ foo: 'bar' })), 'should stringify JSON');
} else {
ok(true);
}
Expand All @@ -159,18 +193,20 @@ test('json: true', 1, function () {
module('delete', before);

test('delete (deprecated)', 1, function () {
var original_cookie = document.cookie;
document.cookie = 'c=v';
$.cookie('c', null);
equal(document.cookie, '', 'should delete the cookie');
equal(document.cookie, original_cookie, 'should delete the cookie');
});


module('removeCookie', before);

test('delete', 1, function() {
var original_cookie = document.cookie;
document.cookie = 'c=v';
$.removeCookie('c');
equal(document.cookie, '', 'should delete the cookie');
equal(document.cookie, original_cookie, 'should delete the cookie');
});

test('return', 2, function() {
Expand Down