cjsm11 loader: detect upgraded modules?

[mk-pmb]

After a while of wondering why require('async') yielded an empty object, I found that it had AMD support and thus used define([], async);, leaving the empty object in module.exports which I probably received at first. Once found, the solution was easy of course: Just not using the cjsm11 loader.

It would be nice however to handle upgrades gracefully, when an author of a previously cjs-only module adds AMD support later on. Would there be a way to make the cjsm11 loader detect the upgrade and prefer the define()d object? Maybe only if module.exports holds the same empty object that it was initialized with?

Yours, MK

[unscriptable]

Hey @mk-pmb,

Very interesting. There must be something about this "async" module that's different from other modules that expose both CommonJS and AMD hooks (aka "UMD" modules). Our flavor of UMD modules can be loaded with the cjsm11 loader without issues. In many cases, the cjsm11 loader will even work if the CJS module has been pre-translated to AMD!

tl;dr: there's something special about this "async" module.

Since there are many, many flavors of UMD, it's hard to make all of them Just Work(tm) with cjsm11. Sorry you had to debug this hard-to-find problem. :(

Maybe only if module.exports holds the same empty object that it was initialized with?

Hm. That won't work. The whole purpose of module.exports is to assign a new value to exports. It's almost always different from what was provided by the module system.

I'm going to close this issue for now since I can't see a way to make this work universally -- unless you've got another idea! Please feel free to keep commenting, though.

Regards,

-- John

[mk-pmb]

async = https://github.com/caolan/async/blob/master/lib/async.js The part you quoted was for cases where the module does define() something as well as modifying module.exports, in that case you might want to keep the old behavior of the cjsm loader and deliver the module.exports instead of the define()d object. (No known benefit, just for conservation of functionality.)

async, when it sees and AMD loader, only define()s. which seems to be ignored, as the current cjsm loader prefers to return the (unmodified, empty) m.e.

however, checking the resulting m.e for both whether it's the same object as initialized is not enough, as other modules just add properties to that, still the same object. so it would have to be combined with an emptyness check. sounds like quite some overhead, where it should suffice to just prefer any define()d object.

Yours, MK

[mk-pmb]

Another easy fix should be to hide "define" for the CJS module, so it would have to use module.exports as intended. wrapSource could just shadow it by prepending var define = undefined; ... or so I thought. Testing it revealed that even though async.js then does go the Node.js route (line 950) and does assign m.e, the result of require('async') still is an empty object. I'm puzzled for the moment, will add my findings if I discover more. Yours, MK

[unscriptable]

I really like your idea to hide define. This effectively forces CJS-node over amd. +1

The fact that you're getting an empty object could be caused by a circular dependency. Any chance you've got a circle?

-- John

Sent from planet earth

On Feb 8, 2014, at 4:18 AM, mk-pmb notifications@github.com wrote:

Another easy fix should be to hide "define" for the CJS module, so it would have to use module.exports as intended. wrapSource could just shadow it by prepending var define = undefined; ... or so I thought. Testing it reveald that even though async.js then does go the Node.js route and does assign m.e, the result of require('async') still is an empty object. I'm puzzled for the moment, will add my findings if I discover more. Yours, MK

— Reply to this email directly or view it on GitHub.

[mk-pmb]

I fail to see how async could be part of a circular dependency, as I cannot find any require( in it. To verify that async chose the Node.js branch of its export method detector, I added a call to my debug logger (update: and two test props)

     // Node.js
     else if (typeof module !== 'undefined' && module.exports) {
         module.exports = async;
+        window.onerror('async=node ' + Object.keys(module.exports));
+        exports.exportedProperty = true;
+        exports = { replacedExports: true };
     }

and from this line inside, the logger receives a list of all functions that I expect, while the require()ing script only gets an empty object. Update: Now it gets an object with one property, exportedProperty, I think with this you can conclude most of it.

Edit 2: Above results occured under the influence of var define = undefined; added into the string concat inside function wrapSource before source. In theory, the = undefined part of it should be mere redundancy.

HTH, MK

[unscriptable]

Hey @mk-pmb,

I'm implemented your most excellent var define; fix in the dev branch. That should fix the issue with async.js.

In your most recent test, I would expect that async would be exported correctly, despite all of fiddling with exports after assigning module.exports. curl.js uses the value that is assigned to module.exports (which is initiallymodule.exports = exports;).

Feel free to double-check with the dev branch.

-- John

[unscriptable]

OT: have you considered Promises, rather than callbacks?

[mk-pmb]

I use promises a lot, even when.js. How to make my own I actually learned from the cujojs tutorial. So thanks to all cujo team for that! :-)

Now testing with dist/curl-for-jQuery/curl.js and src/curl/loader/cjsm11.js both from dev branch. -> checkToAddJsExt error as in cujojs/curl#252, will re-apply b3dd144e2288fe46ae613014b5ae0daf0fe488ac. require('async') yields the exportedProperty only.

Testing with dist/curl-kitchen-sink/curl.js from dev branch, without loader/cjsm11.js:

Error: Syntax or http error: 3rd-party/cujojs/loader/cjsm11.js

So the kitchen sink seems to not include the CJSM loader. With loader/cjsm11.js: exportedProperty only.

I also re-arranged the lines inserted into async.js, so now the debug log message is issued last of the three added lines. Even there it verifies that module.exports still holds all the functions I'd want from async. Which btw also shows that hiding define had worked. :-)

Yours, MK

[unscriptable]

Ok, I see the [other] problem. You may be the very first person to try to use the dist/ versions of curl with the cjsm11 loader extension! That's a scenario for which we have no tests, and google closure compiler ensured that it couldn't ever have worked. (Thanks, google. I find it strange that the compiler doesn't detect module.exports -- or maybe it just doesn't detect contextObj.module.exports?)

Anyways, adding a fix and a test now.

-- J

[unscriptable]

Try the dev branch now. If it works for you, I'll tag this as 0.8.9 and likely release tomorrow. Thanks for your help! -- John

[mk-pmb]

Thanks, will test asap! Also, we should change the undefined/hiding define to being a param instead of a var def in order to not degrade a possible "use strict"; from being the first thing inside the function. Yours, MK

[unscriptable]

Oh man, good call on "use strict";! I was thinking of adding ;var define to the end of the source to cut down on the leading boilerplate, but your suggestion is less ugly and smaller.

[unscriptable]

Pushed. http://git.io/nWRx2w

[unscriptable]

I found a problem with this approach. Adding parameters to the factory causes the cram plugin to offset all imported modules by 1! I'll try to fix this tonight.

[unscriptable]

Released in curl.js 0.8.9 and cram 0.8.0

[mk-pmb]

I have tried with current dev branch's kitchen sink bundle and jquery bundle, unable to reproduce the error with any of them, so it probably works. Thanks! :-) Edit: I also tried both with and without the cjsm loader with both packs, so four cases in total, each time the require()ing module sees all the functions I expect from async.

[unscriptable]

Thanks @mk-pmb!