Helpful error message when undefined passed as cookie name

[rattrayalex]

Hey there,

Totally an unnecessary nice-to-have, but right now TypeError: Cannot read property 'replace' of undefined is thrown from within the library if undefined is passed as the name.

Not too hard to be debug but would be awesome to instead see "Cannot set cookie with blank name" or something like that.

No worries either way; thanks for a great library!

Cheers, Alex

[voltace]

Thanks for your feedback Alex!

Adding input validation seems like a fair request, could do something like:

  // Input validation
  if (!(typeof name == "string" || name instanceof String)) {
    throw 'cookie name is not a string'
  }
  if (!name) {
    throw 'cookie name is empty'
  }
  if (!(typeof value == "string" || value instanceof String)) {
    throw 'cookie value is not a string'
  }

Adding the input validation listed above increases the (minified and gzipped) size of browser-cookies by 73 bytes to 596 bytes. Not a large increase in absolute terms, though given the goal of being a tiny cookies library the size increase is important to take into account nevertheless.

If input validation is not a must-have or should-have I would prefer not add the functionality to keep the file size low, is this acceptable for you?

[rattrayalex]

Hi there! Thanks so much for your thoughtful response! I think that looks like a great solution though I can't speak to the tradeoff -- I certainly appreciate how small the library is, but wouldn't mind the extra 70 bytes or so. I'd leave it up to others or yourself as to what the best way to go is.

Maybe something smaller like if (!(name && typeof name === 'string')) throw 'Cookie name must be nonblank string' could help. But really up to you :-)

[voltace]

I will look into adding the input validation.

As for the file size I didn't manage to get it any smaller:

• All unnecessary syntax and white space is removed by minifying
• Combining the two errors for the cookie name actually increased the gzipped size (due to gzip de-duplication)
• The check on whether the variable is of the type string needs to unfortunately use both typeof and instanceof because strings can also be created as object:

typeof 'cookie name here' === 'string' // true
'cookie name here' instanceof String // false

typeof new String('cookie name here') === 'string' // false
new String('cookie name here') instanceof String // true

To keep the file size in check I won't add any input validation on the options (cookie path, date, etc) for now.

A disadvantage on checking for string type is that it may break existing client code, for example it's currently possible to use a number as cookie name. Perhaps the next release should be numbered 2.0.0 or 1.1.0 instead of 1.0.9 to warn users against the API being enforced more strictly?

[rattrayalex]

Wow, nice!

I'm not sure what the right call there is then. If numbers work as cookie names they certainly shouldn't be invalidated.

Perhaps leaving it without the validation is fine; after all, googling the error will now hopefully lead to this github issue.

[voltace]

True, I'll leave the validation out then. Thank you @rattrayalex for your help.