Fix CommonJS issues for contexts without module global
#63
Conversation
This pull request fixes issues for CommonJS contexts that doesn't provide the `module` global by implementing a slightly tweaked version of https://github.com/umdjs/umd/blob/master/commonjsStrictGlobal.js The adjustments made are just to pass in the expected `root` to still allow the jQuery version to work.
| if (typeof define === 'function' && define.amd){ | ||
| // AMD. Register as an anonymous module. | ||
| define(['exports'], function (exports, b){ | ||
| factory((root.PubSub = exports), b); |
There was a problem hiding this comment.
I'm not following theses changes, doesn't this mean that you're also exporting the PubSub object as a global here. Also why is the "exports" module needed? Is this something supported by all AMD implementations?
What was wrong with the factory returning the PubSub object?
There was a problem hiding this comment.
doesn't this mean that you're also exporting the PubSub object as a global here.
It does
Also why is the "exports" module needed? Is this something supported by all AMD implementations?
That's a good question. It's supported by RequireJS, don't know about others.
What was wrong with the factory returning the PubSub object?
Just consistency, since for some environments, the return value was not used
There was a problem hiding this comment.
doesn't this mean that you're also exporting the PubSub object as a global here.
It does
I think that goes against the idea of AMD allowing you to keep the global namespace clean. What's the rationale behind this? If someone wants a global they can always export it themselves? But this could cause problems in places where AMD is used to avoid polluting the global namespaces, extensions and bookmarklets lets being a good example.
There was a problem hiding this comment.
exports is part of the AMD spec AFAICT from https://github.com/amdjs/amdjs-api/blob/0e057fc77e6bd3522586ffd79e8ab0c5a3285b93/AMD.md#dependencies-
There was a problem hiding this comment.
I think that goes against the idea of AMD allowing you to keep the global namespace clean. What's the rationale behind this? If someone wants a global they can always export it themselves? But this could cause problems in places where AMD is used to avoid polluting the global namespaces, extensions and bookmarklets lets being a good example.
My work was based on https://github.com/umdjs/umd/blob/master/commonjsStrictGlobal.js ... I'll update it to use https://github.com/umdjs/umd/blob/master/commonjsStrict.js
Use the [commonjsStrict](https://github.com/umdjs/umd/blob/master/commonjsStrict.js) pattern instead of [commonjsStrictGlobal](https://github.com/umdjs/umd/blob/master/commonjsStrictGlobal.js), so we don't introduce a new global.
|
@aron let's try this again :) |
|
⛵ |
Fix CommonJS issues for contexts without `module` global
|
Cheers |
This pull request fixes issues #49 and #51 in CommonJS contexts that doesn't provide the
moduleglobal by implementing a slightly tweaked version of https://github.com/umdjs/umd/blob/master/commonjsStrictGlobal.jsThe adjustments made are just to pass in the expected
rootto still allow the jQuery version to work.