Fix the export in CommonJS environments

[oyvindkinsey]

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

[ghost]

Thank you! I'll merge this in tomorrow—it's been a busy day.

[jdalton]

@kitcambridge

Take a look at how Lo-Dash, Benchmark.js, and other BestieJS projects do it: https://github.com/bestiejs/lodash/blob/v0.4.2/lodash.js#L3773-3802

You will want to check for exports after define in case a build optimizer, like r.js, adds an exports object.

[jdalton]

I would say this is a won't fix issue as the define.amd has to be supplied and should not be in a CommonJS enviro. Also some Node modules, like r.js, add define.amd so your AMD code will work as expected in Node. Searching for module first would nix that too.

[oyvindkinsey]

I guess there's really no way out of this - when used in a mixed environment, which all pages by default is, these 'universal' export blocks are bound to fail as they cannot differentiate between local and global scope... This is why I would rather have the build create separate versions..

[jdalton]

@oyvindkinsey Don't create define.amd unless you want code to use it.

[oyvindkinsey]

That's a moot point, I'm not always in control of the environment where my code runs, CJS can easily be used internally in a library as opposed to being the top level service locator.

[jdalton]

@oyvindkinsey

That's a moot point

No it's not, it is THE point. If your enviro has define.amd supplied then it is opting into AMD. Solutions to popular libs that may have multiple versions on the page or included as widgets is to expose it to the global as well as the AMD module and then allow something like a noConflict method to remove it from the global when needed.

[jdalton]

@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 noConflict method to avoid that issue of having multiple versions on a page (some using AMD and others not).

[ghost]

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?

You may also want to add a noConflict method to avoid that issue of having multiple versions on a page (some using AMD and others not).

Why would one need to load multiple versions of a polyfill on a page?

[jdalton]

@kitcambridge

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.

Lo-Dash attempts to use native Function#bind and Object.keys so not too different.

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?

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.

You may also want to add a noConflict method to avoid that issue of having multiple versions on a page (some using AMD and others not).

Why would one need to load multiple versions of a polyfill on a page?

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).

[ghost]

Lo-Dash attempts to use native Function#bind and Object.keys so not too different.

Right, but it doesn't overwrite Function#bind and Object.keys if they don't exist and/or are buggy. :smiley:

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).

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 stringify and parse if it has already been included.

[oyvindkinsey]

So why was it not ok to switch the order of the export clauses? It is much more lightly to have a globally defined define and a locally defined exports than vice versa, and hence, the most lightly scenario should be catered for. Regarding implicitly adding to global, this is simply a bad idea, even if it does fix a broken implementation. Thing is, you have no way to reason about whether that will actually fix issues, or cause issues. So better leave it.

I would rather keep the implementation without loaders, and use buildstep to wrap it in the appropriate closure for each environment.

[jdalton]

So why was it not ok to switch the order of the export clauses?

I cover it in the code comments. AMD build optimizers may add their own exports object.

It is much more lightly to have a globally defined define and a locally defined exports than vice versa, and hence, the most lightly scenario should be catered for.

Some environments may have define but less would have define.amd.

Regarding implicitly adding to global, this is simply a bad idea, even if it does fix a broken implementation. Thing is, you have no way to reason about whether that will actually fix issues, or cause issues. So better leave it.

jQuery exposes itself to the global too when loaded as an AMD module too. If you have a noConflict() there is no problem. jQ has a massive market share and hasn't had problems.

[jdalton]

@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).

[ghost]

JSON.noConflict()? Ugh...