googlearchive / observe-js

A library for observing Arrays, Objects and PathValues
1.35k stars 116 forks source link

If variable exports is in global scope, Observer, ArraySplice, etc don't get put in global scope #81

Closed polpo closed 9 years ago

polpo commented 9 years ago

At issue is this code:

  if (typeof exports !== 'undefined') {
    if (typeof module !== 'undefined' && module.exports) {
      expose = exports = module.exports;
    }
    expose = exports;
  } 

If there is an exports global, expose gets assigned to that global. It looks like an attempt at putting observe-js into modules.exports when used as a Node.js module, which is good, but it doesn't actually work.

I was bit when I had a <div id="exports"> on my page. This created a global named exports (thanks to this crazy "feature"), so this code thought I was running node and stuffed the API into the exports global.

Perhaps this is a symptom of #66 - this stuff shouldn't be put into globals anyway.

jmesserly commented 9 years ago

I seem to recall whoever submitted the pull request for "exports" said it was a common pattern used by other JS libs. Happy to tweak this according to whatever the "flavor of the month" is for JavaScript libraries. Is there some pattern we can follow? (and seriously, ES6 modules cannot come fast enough ... I want them yesterday ... or maybe 10 years ago :) )

(edit: for clarity)

polpo commented 9 years ago

This is an easy enough fix (using just module.exports and not exports), but fixing would actually break any code that uses observe-js in Node.js since it'd have to use the Observer global.

It would also break platforms like NW.js that combine Node.js and the browser. For instance I'm running Polymer in an NW.js project. Polymer uses observe-js's globals, but if the code detects that it's running on Node.js and puts everything in module.exports, then Polymer will break.

polpo commented 9 years ago

I'm fine with submitting a PR that puts things both in the global scope and module.exports if it exists. Doesn't break current code and behaves a bit nicer when being used as a Node.js module.

jmesserly commented 9 years ago

happy to take a pull request too, if you know what the best fix is. Here was the original change that added this: https://github.com/Polymer/observe-js/commit/ffdfb18c783acf98b721907d6e73ce43a92b163b

polpo commented 9 years ago

I take back my comment that it doesn't work under Node as-is. I'll simply add some checks that the exports smells like it is when observe-js is being require()d as a Node module.