Skip to content
This repository has been archived by the owner on Mar 13, 2018. It is now read-only.

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

Closed
polpo opened this issue Jan 28, 2015 · 5 comments · Fixed by #82

Comments

@polpo
Copy link
Contributor

polpo commented Jan 28, 2015

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.

@polpo polpo changed the title If variable exports is in global scope, Observer, ArraySplice, etc don't get put in global scope If variable exports is in global scope, Observer, ArraySplice, etc don't get put in global scope Jan 28, 2015
@jmesserly
Copy link
Contributor

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
Copy link
Contributor Author

polpo commented Jan 28, 2015

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
Copy link
Contributor Author

polpo commented Jan 28, 2015

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
Copy link
Contributor

happy to take a pull request too, if you know what the best fix is.
Here was the original change that added this: ffdfb18

@polpo
Copy link
Contributor Author

polpo commented Jan 29, 2015

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants