Fix the export in CommonJS environments #14
Conversation
Summary: If the current module is used in a mixed environment, loaded as part of a CommonJS style module, but where the scopechain contains an AMD style 'defined' function, this function would wrongfully use this to define itself instead of exporting using the module/exports variables. This diff changes the order of the tests so that it prefers exporting to the `module` object, then the `exports` object and finally using `define`. Test Plan: Ran the test suite, and loaded the module inside a local CommonJS environment running in a page also hosting an AMD style loader. Reviewers: kitcambridge
Thank you! I'll merge this in tomorrow—it's been a busy day. |
module.exports = JSON3; | ||
} else if (typeof exports !== "undefined") { | ||
// Node style, but only the exports object is available | ||
JSON3 = exports; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I merge the module
and exports
branches (since exports
is already an object, and module.exports == exports
by default)—or is it important to prefer module
to exports
here?
Take a look at how Lo-Dash, Benchmark.js, and other BestieJS projects do it: You will want to check for |
I would say this is a |
I guess there's really no way out of this - when used in a mixed |
@oyvindkinsey Don't create |
That's a moot point, I'm not always in control of the environment where my |
No it's not, it is THE point. If your enviro has |
@kitcambridge JSON3 should be patched to mimic Lo-Dash to ensure compat with AMD build optimizers. The code comments explain it in detail. You may also want to add a |
What complicates this discussion further is the fact that JSON 3 is not a traditional library—it's a polyfill. When loaded in a CommonJS environment or with an AMD loader, it still delegates to the native implementations, but doesn't overwrite any globals. Unless there's a compelling use case for loading a polyfill with a module loader, we may want to consider axing AMD support entirely. Thoughts?
Why would one need to load multiple versions of a polyfill on a page? |
Lo-Dash attempts to use native
You are loading an object with methods which can be used in modules (including AMD), there is no reason why it shouldn't be supported.
This is more for the uncontrolled use like when JSON3 is compiled into a widget that's included on a page that happens to also use AMD (so JSON3 used as a global and as an AMD module). |
Right, but it doesn't overwrite
Ah. In that case, I still think we should pave the global methods if they're broken, in addition to exporting for AMD loaders. JSON 3 passes its own feature tests, so it shouldn't overwrite |
So why was it not ok to switch the order of the export clauses? I would rather keep the implementation without loaders, and use buildstep to wrap it in the appropriate closure for each environment. |
I cover it in the code comments. AMD build optimizers may add their own
Some environments may have
jQuery exposes itself to the global too when loaded as an AMD module too. If you have a |
@kitcambridge if it's loaded as a module I don't think it should pave the global methods (excluding the AMD case but there should be a mechanism to revert the global exposure). |
|
Summary:
If the current module is used in a mixed environment, loaded as part of
a CommonJS style module, but where the scopechain contains an AMD style
'defined' function, this function would wrongfully use this to define
itself instead of exporting using the module/exports variables.
This diff changes the order of the tests so that it prefers exporting to
the
module
object, then theexports
object and finally usingdefine
.Test Plan: Ran the test suite, and loaded the module inside a local
CommonJS environment running in a page also hosting an AMD style loader.
Reviewers: kitcambridge