ScottHamper / Cookies

JavaScript Client-Side Cookie Manipulation Library
The Unlicense
1.77k stars 170 forks source link

Expired cookie set to "undefined", not `undefined` #37

Closed jamesplease closed 9 years ago

jamesplease commented 9 years ago

Type: Bug

Description: Expiring a cookie sets its value as a string, "undefined".

What I would expect: The value would be the 'default' value for cookies, or, undefined. This is important for libraries that check for the existence of a cookie by its value, which is important given that there is no other method to check for existence.

Cause: I suspect that this is what happens: Cookies.expire delegates to Cookies.set, passing undefined as the value. That method then delegates to _generateCookieString to compute the actual value to set. L88, within that method, is the likely culprit. undefined + "" gets converted to "undefined".

If you're interested in a fix, I can whip up a PR.

Temporary Fix: Fix the value of any cookie in your code after retrieving it.

var cookie = Cookies.get('cookieName');
if (cookie === 'undefined') { cookie = undefined; }
jamesplease commented 9 years ago

nevermind, this works fine in the browser. It seems to be an issue with JSDom.

ScottHamper commented 9 years ago

Interesting issue - it sounds like JS Dom is ignoring the "expires" option that gets set.

In reality, Cookies.js does set the cookie value to "undefined" - but it also sets "expires" to be one second in the past. Setting the date in the past causes browsers to immediately delete the cookie, so it no longer matters what the cookie value is. JS Dom just might not be respecting this expiration mechanic.

jamesplease commented 9 years ago

JS Dom just might not be respecting this expiration mechanic.

That could very well be the case! This issue is likely related to the discussion here, https://github.com/tmpvar/jsdom/issues/310, where it sounds like cookies are still being worked on in JSDom.

I don't have the time to investigate it further, so (for the record), I'm getting around it by including following snippet in my node-only test setup:

var cookies = require('cookies-js');
var originalGet = cookies.get;
cookies.get = function(cookieName) {
  var value = originalGet(cookieName);
  return value === 'undefined' ? undefined : value;
};