ScottHamper / Cookies

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

using the latest jsDom returns undefined for Cookies definition #57

Closed kwhitaker closed 8 years ago

kwhitaker commented 8 years ago

It looks like the later versions of jsDom aren't playing nicely with Cookies. Per your example, I've tried the following:

import {jsdom} from 'jsdom'

let Cookies
if (process.env.NODE_ENV === 'test') {
  Cookies = require('cookies-js')(jsdom().defaultView)
} else {
  Cookies = require('cookies-js')
}
export default Cookies

Cookies.method() work as expected in production and development, but Cookies is undefined when I run my unit tests. Is there something I'm missing?

Thanks

ScottHamper commented 8 years ago

Hey Kevin,

Can you see if Cookies.js throws an error when executing:

require('cookies-js')(jsdom().defaultView)

I have a feeling the issue is related to what the value of jsdom().defaultView is. If it does not have a document object property, Cookies.js will throw the following error:

"Cookies.js requires a window with a document object"

This will result in your Cookies variable being undefined. Outside of exceptions being thrown, or require suddenly not working, Cookies should never be undefined.

kwhitaker commented 8 years ago

@ScottHamper It doesn't throw an error, and this is what console.log(jsdom().defaultValue) returns: Document { location: [Getter/Setter] }.

I added a logging statement to cookies.js to verify, and it returns the same thing.

kwhitaker commented 8 years ago

Ah... it looks like I found the issue. I'm using http://airbnb.io/enzyme/docs/guides/jsdom.html, and this code:

var jsdom = require('jsdom').jsdom;

var exposedProperties = ['window', 'navigator', 'document'];

global.document = jsdom('');
global.window = document.defaultView;
Object.keys(document.defaultView).forEach((property) => {
  if (typeof global[property] === 'undefined') {
    exposedProperties.push(property);
    global[property] = document.defaultView[property];
  }
});

global.navigator = {
  userAgent: 'node.js'
};

seems to play hell with what cookies expects. Commenting it out solves the problem with cookies, but opens up other issues. Off-hand, can you think of a modification I could make to satisfy both?

ScottHamper commented 8 years ago

So it looks like since your global object already has a window property (from using jsdom), Cookies.js ends up automatically executing its factory function using that window property, and then exports the result.

Basically, Cookies.js checks if there's a window property on the global object. If there isn't, it exports a factory function that will accept a window object and return the Cookies API bound to that window. If there already is a window property on the global object, Cookies.js won't export the factory function - it will auto-execute it using the global window, and then export the Cookies API directly.

This export behavior is inconsistent depending on the environment Cookies.js is run in, which I dislike, but it was a concession so that using Cookies.js in the browser didn't require the developer to explicitly pass in the window object (the goal was to reduce boilerplate code in the default/majority scenario of using the library in the browser).

In other words, looking at the code in your original post, try removing the conditional and always execute the following, regardless of environment:

Cookies = require('cookies-js')

Since you're running the copy/pasted code in your post above to make the node environment global object directly emulate the browser global, there's no need to do anything special/conditionally for Cookies.js to work. Just use it as if it were still running in a browser.

If I ever get around to rewriting the library (to update to ES6 or TypeScript), I will likely make the export behavior consistent regardless of environment.

kwhitaker commented 8 years ago

That did it. Thanks so much.

ScottHamper commented 8 years ago

Great! Glad you got it working.