voltace / browser-cookies

Tiny cookies library for the browser
The Unlicense
89 stars 18 forks source link

Feature node shim #3

Closed bengreenier closed 9 years ago

bengreenier commented 9 years ago

@voltace i'm using this module for a javascript "client" module that's consumed both by node and the browser. In the browser, i use browser-cookies to add a feature, but in node i do not.

currently, require('browser-cookies') in node results in a failure.

this change simply determines if the library is being used in node (using the same method underscore uses) and if so, shims out the methods to do nothing.

voltace commented 9 years ago

@bengreenier If I understand you correctly this pull request should allow browser-cookies to be able to run on node by shimming all methods, so client javascript can be executed on node without any errors.

However, wouldn't the proposed code change also cause browser-cookies to be shimmed when using browserify? Both browserify and node use module.exports so I would expect the contents of the if-statement to be executed on both node and browserify:

exports.set = function(name, value, options) {
  if (typeof module !== 'undefined' && module.exports) {
    return;
  }
  ...
bengreenier commented 9 years ago

i want to agree, however i'm not 100% sure why, but when using my forked version of browser-cookies in a a browserify entry point, it works fine. I can require('my-module') in the browser and use browser-cookies from within my-module without issue, and i can require('my-module') in node and get the shimmed version of browser-cookies.

For good measure though, i've added a check for process.browser which is supposedly set by browserify and we do not shim when that is present.

voltace commented 9 years ago

Ok than it seems process.browser will do.

I'm trying to come up with other use cases to run browser-cookies on node, and perhaps some design alternatives. My current thoughts:

A) A client might want to actually get/set/erase cookies on node instead of shimming, for example by using a DOM that supports cookies like jsdom. This would probably require some changes in browser-cookies as well.

B) Not including any shimming functionality could be an option, especially since a primary design goal for browser-cookies is to keep the file size down to a minimum. For example I would prefer not adding any functionality that isn't related to basic cookies parsing (e.g. no automatic JSON encoding/decoding). In shared client/server javascript a simple if statement would be sufficient:

var cookies = require('browser-cookies');
if (process.browser) {
   cookies.set('name', 'value');
}

C) The accepted answer in the stackoverflow question from your previous post suggests using the browser field to use a different library for the client and server. I could add a browser-cookies-shim package for this purpose. This would mean your project would require('browser-cookies-shim') in the code and add the following section to the package.json:

{
  "browser": {
    "browser-cookies-shim": "browser-cookies"
  }
}

I'm guessing A isn't really relevant to your current situation. What are your thoughts on the matter, would either B or C be acceptable to you?

bengreenier commented 9 years ago

B or C seems sufficient, i prefer C to B, as that method doesn't require any code changes, and doesn't require any additional work by developers (other than the package.json change) to implement.

FWIW, the reason i choose to implement this as a shim was simply because I feel anything that can be npm install-ed should work "out of the box" in node. That being said, i'm sure browser-cookies isn't the first module that doesn't work with node.

voltace commented 9 years ago

Than C it is!

voltace commented 9 years ago

Created browser-cookies-shim and added the package to npm: https://github.com/voltace/browser-cookies-shim https://www.npmjs.com/package/browser-cookies-shim

A small unit test and installation instructions are included, though I've yet to verify whether adding the following snippet to a package.json file will result in browsers using the real package instead of the shim:

{
  "browser": {
    "browser-cookies-shim": "browser-cookies"
  }
}
bengreenier commented 9 years ago

cool. closing this pr then, as that should solve the issue. will test and open any issues with browser-cookies-shim if i find anything. thanks for the awesome work! :+1: