ScottHamper / Cookies

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

Check for existence of "document" and "navigator" before referencing them #28

Closed zertosh closed 9 years ago

zertosh commented 9 years ago
ScottHamper commented 9 years ago

Hey Andres,

I'm not sure I understand the situation - would you be able to clarify how you're using Cookies.js that causes an issue, and how this commit resolves the problem?

Specifically, I don't understand why having Cookies._document and Cookies._navigator set to null is preferable to having them be set to undefined.

ScottHamper commented 9 years ago

Derp, I get it. I'm forgetting my JS...

It'll throw an error if document and navigator aren't defined, it won't just magically set Cookies._document and Cookies._navigator to undefined.

I'd still like to hear more about your specific use case, though, if possible! I haven't dabbled with putting client side libraries into server side/headless run times, so it's something I'm not familiar with.

zertosh commented 9 years ago

I've got two different use cases:

  1. I've got modules that are shared between the client and the server, where the code paths that are used on the server don't make use of Cookies. However, because other functions do, I've got to wrap my require call to cookies-js in an if to check for the existence of document before actually requiring it, otherwise it blows up. The if isn't so bad, but just requiring a module should not cause it to throw. It should throw if you actually try to use it without a document.
  2. For testing some of those modules in a "browserless" environment, it'd be nice to be able to require cookies-js and then pass it in a DOM. Think of it as stubbing a DOM. In my case, I'd be doing something like this in node:
var Cookies = require('cookies-js');
var jsdom = require('jsdom');
var window = jsdom.jsdom().parentWindow;

Cookies._document = window.document;
Cookies.enabled = Cookies._areEnabled();

// Use Cookies as you would on a Browser
ScottHamper commented 9 years ago

Ok, thanks!

I understand the situation now. However, I'd like to implement a solution similar to what jQuery does, which is to expose a factory method that takes in the window for CommonJS/Node.js environments that do not inherently have a window object. The resulting API would look something like:

var jsdom = require('jsdom');
var window = jsdom.jsdom().parentWindow;
var Cookies = require('cookies-js')(window);

This way, consumers of the library don't have to muck around with undocumented/non-public API features in order to get things to work well in a browser-less environment.

I also discovered that navigator is no longer actually used by the library, and can be removed.

As a reference, here's how jQuery handles this issue: https://github.com/jquery/jquery/blob/2.1.1/src/intro.js

Let me know if you have any concerns with this approach.

ScottHamper commented 9 years ago

Hey Andres,

I've implemented the solution described in my last message. Would you be willing to try it out in your environment? Just grab the latest file from master.

In environments that have a window present, require('cookies-js') will work as it always has and return the Cookies object. In environments that do not have a window present, require('cookies-js') will return a factory method that accepts a window object.

Thanks for contributing to the project!

zertosh commented 9 years ago

@ScottHamper, it works great! thanks! can you publish the update to NPM?

ScottHamper commented 9 years ago

Excellent!

Published!