Fix issue when parsing invalid cookies

[danielmcq]

When an invalid cookie string is encountered, it is ignored and skipped.

Fixes #33.

[andyburke]

What kind of knock-on effect is there to return nothing in this case?

Should we throw instead?

[danielmcq]

It currently does throw an error, although not a very helpful one. I think this library gets used in a lot of other libraries (https://www.npmjs.com/package/cookiejar?activeTab=dependents) and potentially runs across invalid cookie headers from lots of different sites. My assumption is that most of the people that end up using this library just want it to work, but without errors. However, I do feel like there should be some way to know that you are getting bad cookie headers. A possible solution could be to enable a strict mode.

[andyburke]

I like not blowing up on a bad cookie, now that you mention it. Feels wrong to just ... ignore it, though.

maybe we add a console.warn()?

[danielmcq]

console.warn() definitely seems like a good compromise. Let me update the PR real quick.

[danielmcq]

@andyburke How does that look? I did make use of string templates to show the invalid cookie header in question in the warning message. I'm not sure what JavaScript features this library is aiming to be compatible with, so the string templates might need to change.

EDIT: Removed string templates and made sure that no warnings came from jshint.

[andyburke]

this should probably be:

if ( typeof key === 'undefined' || typeof value === 'undefined' ) {

Otherwise, if you have a false-y value like 0 we'll blow up.

[andyburke]

I guess !'0' actually evaluates false. Is an empty string a valid value per the spec?

[andyburke]

2. If the name-value-pair string lacks a %x3D ("=") character,
   ignore the set-cookie-string entirely.

3. The (possibly empty) name string consists of the characters up
   to, but not including, the first %x3D ("=") character, and the
   (possibly empty) value string consists of the characters after
   the first %x3D ("=") character.

4. Remove any leading or trailing WSP characters from the name
   string and the value string.

5. If the name string is empty, ignore the set-cookie-string
   entirely.

So we should probably do:

if ( typeof key !== 'string' || key.length === 0 || typeof value !== 'string' ) {

[danielmcq]

Yeah. Empty strings are falsy. The string '0' is also falsy. Everything extracted via the regex would be a string, unless we were to try to cast it. https://developer.mozilla.org/en-US/docs/Glossary/Falsy

[andyburke]

I think we should avoid string templates for now. I'm kind of thinking of trying to roll a handful of these PRs into a minor release, then revisit your other PR for a major, at which point we could consider what versions we want to support.

[danielmcq]

Yeah, changing supported JavaScript features would be best for a major version. The other PR (#30) isn't one that I made, but I commented on and agree with @Smert's reasons for doing it. As far as supporting ES6 features, I think that would be fairly safe in the next major version since they've already EOL'd versions of Node.js older than 6 (https://github.com/nodejs/Release#release-schedule), and that version has ES6 features implemented (https://node.green/#ES2015).