Support various module loaders with MODULARIZE

[dcodeIO]

As discussed in WebAssembly/binaryen#1341 this change adds support for AMD loaders to MODULARIZE-ed outputs.

[kripken]

Thanks!

So, I don't really understand these ecosystems myself. How important is it to support AMD? We haven't gotten a request for this support, but I'm not sure if that means anything... maybe not enough people are using MODULARIZE...

cc @yurydelendik

[dcodeIO]

Not sure how urgent this is for the vast amount of emscripten users, but when I create JavaScript modules, I always make sure that it is a proper UMD module so it "just works". The reason for proposing this here is that AMD-support is gone from binaryen.js after the switch to MODULARIZE, and I'd like to still provide an UMD module using the binaryen.js buildbot that publishes to npm.

Ultimately, I'd guess that AMD will be gone for good at some point in time, with the new import/export syntax modern JavaScript has, but it's still somewhat too early to phase it out, imho.

Nonetheless, no matter how you decide, it's also easy to patch AMD support into the buildbot by simply concatenating one additional line of code.

[yurydelendik]

... I always make sure that it is a proper UMD module so it "just works".

@dcodeIO Indeed, it will be nice to see standard UMD. What will it take to properly wrap emcc generated code with UMD? The proposed fix might have some unwanted effect when a packager will auto-detect the module format and/or replace e.g. typeof require === "function" with constants. (Let me play with the patch to see if it's a concern)

[dcodeIO]

Afaik there is already a check for CommonJS (just changed it to a more standard one in this PR) and there's always a global var. So the only missing thing basically is to check for the presence of the define function with a trueish .amd property and call it with a factory function returning the exports.

[yurydelendik]

I was playing UMD variant proposed here at https://github.com/yurydelendik/old-man-sandbox/tree/master/emcc-6006 . See test.js and result produced by webpack after its consuming the input. Notice that webpack was confused about this AMD format and added some polyfill to the output. I also added "test.old.js" to see old format output and "test.proposed.js" to wrap body in standard UMD format. The latter looks like a better choice.

[dcodeIO]

One issue I see with

if (typeof module === "object" && module.exports)

is that module is also of type object when it's null, resulting in a ReferenceError in such a case. It's certainly unusual that there's a var module = null in the scope here, but possible.

Not sure about the polyfill code being added by webpack. Maybe it's safe enough when just doing:

if (typeof module === "object" && module && module.exports)

[yurydelendik]

The webpack generates different UMD header, see webpack/webpack#5826 . We can use something along these lines of:

(function(root, factory) {
    if(typeof exports === 'object' && typeof module === 'object')
        module.exports = factory();
    else if(typeof define === 'function' && define.amd)
        define([], factory);
    else if(typeof exports === 'object')
        exports["Test"] = factory();
    else
        root["Test"] = factory();
...

[dcodeIO]

I see. Though, this makes the ReferenceError occur when there is either a module = null with any object exports, or exports = null with any non-object module in scope. Not sure how important this is in practice, just mentioning.

Mitigation:

(function(root, factory) {
    if(typeof exports === 'object' && typeof module === 'object' && module)
        module.exports = factory();
    else if(typeof define === 'function' && define.amd)
        define([], factory);
    else if(typeof exports === 'object' && exports)
        exports["Test"] = factory();
    else
        root["Test"] = factory();
...

[yurydelendik]

Not sure how important this is in practice, just mentioning.

If somebody will produce such environment, they will have issues with almost all UMD modules. There is no point to protect us from such use case IMHO.

[dcodeIO]

If somebody will produce such environment, they will have issues with almost all UMD modules.

I agree.

There is no point to protect us from such use case IMHO

Personally I'd say that something that's so easy to mitigate should be mitigated (unless it still confuses webpack, ofc), but that's debatable and I'm fine with both.

[yurydelendik]

Personally I'd say that something that's so easy to mitigate should be mitigated (unless it still confuses webpack, ofc),

It's better to choose something standard (canonical UMD, webpack UMD, etc.), so packagers will not be confused that much and they could provide the best transformation for the generated module.

[dcodeIO]

Alright, let's just use webpack's version then.

[yurydelendik]

(Updated example at https://github.com/yurydelendik/old-man-sandbox/blob/master/emcc-6006/result.js)

The proposed patch looks good to me.

[kripken]

Looks good to me too. Can we add a test for the new functionality? We do test modularize and require in node already, so that should already be covered.

[dcodeIO]

Should be good now incl. a test, wdyt?

[kripken]

Thanks, looks good. Please just add yourself to AUTHORS before we merge (can make the commit message with [ci skip] to avoid running CI).

[dcodeIO]

Sorry, messed up the CI. Not, my, day.

[ncave]

This change unfortunately breaks the import from typescript:

if I change the exports from:
module.exports = %(EXPORT_NAME)s;
back to:
module['exports'] = %(EXPORT_NAME)s;
then it works again.

(emscripten v1.37.34, latest NodeJS, TypeScript, rollup).

[yurydelendik]

@ncave can you provide a little bit more information such as minimal test case to demonstrate the issue? or reason why it breaks?

[dcodeIO]

Did something change with closure externs? The code, as is, relies on externs for module with an exports property to be present.

[ncave]

@yurydelendik Here is what gets generated:
mywasm.js

  ...
  return ModuleFunc;
};
if (typeof exports === 'object' && typeof module === 'object')
  module.exports = ModuleFunc;
else if (typeof define === 'function' && define['amd'])
  define([], function() { return ModuleFunc; });
else if (typeof exports === 'object')
  exports["ModuleFunc"] = ModuleFunc;
export { ModuleFunc };

And how it's being consumed:

import { ModuleFunc } from "../js/wasm/mywasm";

works again, as already mentioned, if exports are changed back to:
module['exports'] = ModuleFunc;

[yurydelendik]

@ncave why there is export { ModuleFunc }; at the end?

[ncave]

@yurydelendik Doh, I was adding that before to make it work, I guess I need to find another way. Thanks a lot, sorry about that.

[yurydelendik]

Small working TS example:

main.ts:

import * as ModuleFunc from "./mywasm"; // CommonJS with "default" export
console.log(ModuleFunc()());

mywasm.js:

var ModuleFunc = function () {
  function ModuleFunc() {
    return "test";
  }

  return ModuleFunc;
};

if (typeof exports === 'object' && typeof module === 'object')
  module.exports = ModuleFunc;
else if (typeof define === 'function' && define['amd'])
  define([], function() { return ModuleFunc; });
else if (typeof exports === 'object')
  exports["ModuleFunc"] = ModuleFunc;

[ncave]

@yurydelendik Thanks, I really appreciate your help, sorry about mucking the PR instead of an opening an issue! Looks like my problem was mainly with rollup.