guard clause in _generateCookieString

[dzincolorado]

Hi Scott,

I'm trying to import your package into our Aurelia.io based Spa application and I had to make a minor change to fix an undefined error I was getting during package injection. I'm noob with pull requests, so please let me know if this isn't the right approach.

Thanks!

Denis

[ScottHamper]

Hey Denis,

Can you describe in more detail how Cookies.js was producing an error in your specific use case? I'm guessing that the error was thrown on line 86:

key = key.replace(/[^#$&+\^`|]/g, encodeURIComponent);

There's two possible scenarios I can think of that would cause this:

1. key was undefined, and attempting to call replace on it threw an error. Since _generateCookieString is only called from Cookies.set, it would mean that Cookies.set was called with an undefined key. In this situation, an error ought to be thrown, as key is a required parameter. Though, the library could potentially be improved by specifically checking for undefined and throwing a more helpful, custom error.
2. key was defined, but not a string, and attempting to call replace on it threw an error.

My guess is that number 2 is happening. I haven't worked with Aurelia, so I'm not sure how it handles module loading, but if it's calling Cookies(window) in a browser environment when it loads Cookies.js, then it's actually calling Cookies.set(window). This would cause the number 2 scenario above to happen.

In browsers, where window is implicitly available, Cookies.js will go ahead and export the main Cookies object. If it's not running in an environment where window is available, it will export a factory method that takes an object to act as window and then returns the Cookies object.

Or I could be completely wrong and there's something else going on! Ultimately, I think the inconsistency of exports depending on environment is a problem. I think it would be more consistent and loosely coupled to always export the factory method. It requires an extra step to use the library in a browser environment, which is a minor bummer, but I think it would be a beneficial change in the end.

In any case, let me know if my hunches are correct or not.

Thanks, --- Scott

[dzincolorado]

Hi Scott,

Thanks for getting back to me.  The error was indeed thrown on line 86 because of an undefined value.  I think your suggestions make sense.  I can create a new pull request.

Sorry it took me so long to respond, was working through some application issues.

Thanks,

Denis

Saturday, March 28, 2015, 15:52 -0600 from Scott Hamper notifications@github.com:

Hey Denis, Can you describe in more detail how Cookies.js was producing an error in your specific use case? I'm guessing that the error was thrown on line 86: key = key.replace(/[^#$&+^|]/g, encodeURIComponent);` There's two possible scenarios I can think of that would cause this:

1. key was undefined, and attempting to call replace on it threw an error. Since _generateCookieString is only called from Cookies.set, it would mean that Cookies.set was called with an undefined key. In this situation, an error ought to be thrown, as key is a required parameter. Though, the library could potentially be improved by specifically checking for undefined and throwing a more helpful, custom error.
2. key was not a string, and attempting to call replace on it threw an error. My guess is that number 2 is happening. I haven't worked with Aurelia, so I'm not sure how it handles module loading, but if it's calling Cookies(window) in a browser environment when it loads Cookies.js, then its actually callingCookies.set(window)`. This would cause the number 2 scenario above to happen. In browsers, where window is implicitly available, Cookies.js will go ahead and export the main Cookies object. If it's not running in an environment where window is available, it will export a factory method that takes an object to act as window and then returns the Cookies object. Or I could be completely wrong and there's something else going on! Ultimately, I think the inconsistency of exports depending on environment is a problem. I think it would be more consistent and loosely coupled to always export the factory method. It requires an extra step to use the library in a browser environment, which is a minor bummer, but I think it would be a beneficial change in the end. In any case, let me know if my hunches are correct or not. Thanks, --- Scott — Reply to this email directly or view it on GitHub .