Emscripten: Document CommonJS/AMD support

[mbasso]

Hi,
first of all thank you for working on emscripten 😄
I've just updated my emscripten version from 1.37.22 to 1.37.36 and all works fine without changes, except 1 command. In my project I use emcc to compile some C++ code in this way:

:white_check_mark: .cpp => .a
:white_check_mark: .cpp => .bc
:white_check_mark: .bc => .asm.js
:x: .bc => .js (and .wasm, using BYNARIEN)

Unfortunately the last step gives me an error. I've tried to change some flags and I noticed that it might be caused by -O3, using -O2 no error is thrown. I'll copy here my command and the error:

emcc
  -O3
  --bind
  --memory-init-file 1
  --llvm-lto 3
  --llvm-opts 3
  --js-opts 1
  --closure 1
  --pre-js prefix.js
  --post-js postfix.js
  -s ALLOW_MEMORY_GROWTH=1
  -s "EXPORTED_RUNTIME_METHODS=['UTF8ToString']"
  -s AGGRESSIVE_VARIABLE_ELIMINATION=1
  -s ABORTING_MALLOC=1
  -s NO_EXIT_RUNTIME=1
  -s NO_FILESYSTEM=1
  -s DISABLE_EXCEPTION_CATCHING=2
  -s BINARYEN=1
  -s "BINARYEN_TRAP_MODE='allow'"
  input.bc
  -o output.js

and here is the error (with EMCC_DEBUG=1):

DEBUG:root:emcc step "asm2wasm" took 0.51 seconds
DEBUG:root:running cleanup on shell code: noPrintMetadata AJSDCE minifyWhitespace
DEBUG:root:running meta-DCE

Stack: Error
    at assertTrue (eval at globalEval (/path/to/emsdk/emscripten/1.37.36/tools/js-optimizer.js:114:8), <anonymous>:59:26)
    at Object.emitDCEGraph (/path/to/emsdk/emscripten/1.37.36/tools/js-optimizer.js:8144:3)
    at /path/to/emsdk/emscripten/1.37.36/tools/js-optimizer.js:8368:14
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (/path/to/emsdk/emscripten/1.37.36/tools/js-optimizer.js:8367:21)
    at Module._compile (module.js:635:30)
    at Object.Module._extensions..js (module.js:646:10)
    at Module.load (module.js:554:32)
    at tryModuleLoad (module.js:497:12)
    at Function.Module._load (module.js:489:3)

undefined:60
    throw msg;
    ^
Assertion failed: undefined
Traceback (most recent call last):
  File "/path/to/emsdk/emscripten/1.37.36/emcc", line 10, in <module>
    python_selector.run(__file__, profile=True)
  File "/path/to/emsdk/emscripten/1.37.36/tools/python_selector.py", line 43, in run
    sys.exit(run_by_import(filename, main) if on_allowed_version() else run_by_subprocess(filename))
  File "/path/to/emsdk/emscripten/1.37.36/tools/python_selector.py", line 13, in run_by_import
    return getattr(importlib.import_module(os.path.basename(filename)), main)()
  File "/path/to/emsdk/emscripten/1.37.36/emcc.py", line 1885, in run
    wasm_text_target, misc_temp_files, optimizer)
  File "/path/to/emsdk/emscripten/1.37.36/emcc.py", line 2423, in do_binaryen
    emit_symbol_map=emit_symbol_map)
  File "/path/to/emsdk/emscripten/1.37.36/tools/shared.py", line 2224, in minify_wasm_js
    js_file = Building.metadce(js_file, wasm_file, minify_whitespace=minify_whitespace, debug_info=debug_info)
  File "/path/to/emsdk/emscripten/1.37.36/tools/shared.py", line 2244, in metadce
    txt = Building.js_optimizer_no_asmjs(js_file, ['emitDCEGraph', 'noEmitAst'], return_output=True)
  File "/path/to/emsdk/emscripten/1.37.36/tools/shared.py", line 2096, in js_optimizer_no_asmjs
    return run_process(NODE_JS + [js_optimizer.JS_OPTIMIZER, filename] + passes, stdout=PIPE).stdout
  File "/path/to/emsdk/emscripten/1.37.36/tools/shared.py", line 131, in run_process
    return run_base(cmd, universal_newlines=universal_newlines, check=check, *args, **kw)
  File "/path/to/emsdk/emscripten/1.37.36/tools/shared.py", line 127, in run_base
    result.check_returncode()
  File "/path/to/emsdk/emscripten/1.37.36/tools/shared.py", line 114, in check_returncode
    raise Py2CalledProcessError(returncode=self.returncode, cmd=self.args, output=self.stdout, stderr=self.stderr)
tools.shared.Py2CalledProcessError: Command '['/path/to/emsdk/node/8.9.1_64bit/bin/node', '/path/to/emsdk/emscripten/1.37.36/tools/js-optimizer.js', '/tmp/tmpIF1UzW/asm-dom.bc.o.js.pp.js.mem.js.jsopted.js.jsopted.js.jsopted.js.jso.js', 'emitDCEGraph', 'noEmitAst']' returned non-zero exit status 1

[kripken]

Any chance you can provide the input bitcode, to reproduce the problem?

Without that, as best I can guess is that since the crash is on

assert(foundAsmLibraryArgAssign); // must find the info we need

then something altered the JS that sets up the wasm imports, into a form we can't recognize. None of your compiler flags seems obviously related to that. Perhaps try removing them, and see if compilation doesn't fail without one of them?

[mbasso]

Any chance you can provide the input bitcode, to reproduce the problem?

Sure, I've just created a repository called emscripten-issue-6442. It contains the prefix.js, postfix.js and two input files, one created using [email protected] (input-1_37_22.bc) and another created using [email protected] (input-1_37_36.bc). I don't know if it can be useful but I've put both in the repo... I've also written a Makefile with 2 commands to test it more easily.
I've created these files from asm-dom project, from branch 0.6.0, but there is the same problem on master.

Perhaps try removing them, and see if compilation doesn't fail without one of them?

I've tried to remove some params... It seems that there's a problem with prefix.js and postfix.js, emcc compiles them correctly with -O2 but gives and error with -O3. Running the command without them and -O3 all works fine

[kripken]

Thanks! Ok, I see the problem here: your pre/post js files put all the code inside a function scope. But that pass expects the emscripten output to be at the top level.

This is definitely not clear in our current docs, and maybe not even in our thinking. But I think we should document this as not what pre/post-js are meant for: the idea with those options is code to add to emscripten to be optimized with the rest of its output. That's different from code that could have been prepended/appended after emscripten runs, that the optimizer doesn't need to know about. But this is a little subtle... hard to document, and harder to provide a better error message for. But maybe we can figure that out.

Separately, I think our MODULARIZE and MODULARIZE_INSTANCE options can achieve what your pre/post-jses do, maybe you can use one of those?

[kripken]

An improvement for the error message is in #6444, but I'd like to find even more to do here, this is confusing stuff.

[mbasso]

Awesome! Thank you.

Separately, I think our MODULARIZE and MODULARIZE_INSTANCE options can achieve what your pre/post-jses do, maybe you can use one of those?

I think that I'll try to use MODULARIZE or MODULARIZE_INSTANCE with a new postfix.
I basically need to export the Module using AMD, CommonJS, Node.js or just as global, but I haven't find a feature that allows me to do that in a different way.

An improvement for the error message is in #6444, but I'd like to find even more to do here, this is confusing stuff.

Do you think that MODULARIZE and MODULARIZE_INSTANCE might export the module directly using these standards? A library built with emscripten and usable from JavaScript probably needs it. This means, for example, that we can use the generated code with webpack without problems. Also, prefix and postfix will be used for other purposes.
What do you think about it? I can open another issue if that makes sense

[kripken]

I don't know enough about AMD, CommonJS etc. myself, but I know there have been some discussions about them. It would be good to support them out of the box if that's possible. Maybe search through the issues a little for those terms, and if there isn't an obvious existing issue, open a new one?

[curiousdannii]

Emscripten already makes CommonJS exports, both with and without MODULARIZE. With MODULARIZE it will also do an AMD export.

[mbasso]

Hi @curiousdannii,
yes, I've just found this PR, but it seems that there is no reference to CommonJS and AMD in the doc (I've read the MODULARIZE option in settings.js). Do you think that we have to update the doc to make it clearer?

[curiousdannii]

A PR for the docs would surely help. (Until recently Modularize was hardly even mentioned, I think it was hidden somewhere odd like WebIDL.)

[kripken]

Good idea, renamed this issue to focus on improving those docs.

[rajsite]

Hmm interesting, hit the same thing trying out the WASM=1 flag after upgrading to 1.37.36. Based on the updated docs in #6444 sounds like our usage was unexpected as well: https://github.com/ni/VireoSDK/blob/master/source/core/vireo.wrapper.js#L30

(we have a script to split that file during build to create a pre and post js)

The thing that I'm not a fan of with the current MODULARIZE=1 behavior is that it exposes globally a way to construct the Emscripten Module instance when I would prefer to wrap that privately and give a better API for users. Is the best way to support that by modifying the generated .js file after build as suggested in the MODULARIZE doc?

[kripken]

Yes, modifying it or wrapping it after emcc runs is the best way to do that.

I hope we can find a way to make this more clear and natural. How about if we had 4 options,

• --pre-js, --post-js
• --external-pre-js, --external-post-js

where the first two are "internal" and optimized with the main output, and the last two are "external" and added after all optimization? This would let us refer people to the --external-* options when relevant.

I worry though that adding more options might make it more confusing...

[curiousdannii]

I think it would be better to just recommend people use Browserify or Webpack. And in the future make MODULARIZE be on by default.

Ideal changes IMO (would be breaking changes):

• MODULARIZE on by default
• Fix #5820, probably through returning a real promise
• Possibly remove onRuntimeInitialized so as to reduce redundant alternatives
• Add a flag to switch from UMD to ES6 module exports #6284

[mbasso]

Fix #5820, probably through returning a real promise

I've just faced this issue too and I had to manually delete .then to resolve a Promise with the instance of my library:

import('../library.js').then(Module => {
    const lib = Module(config);
    delete lib.then;
    return lib;
});

MODULARIZE on by default

I think that it would be great to enable it by default, new users would be ready to use the generated code without configuration

I worry though that adding more options might make it more confusing...

Maybe we can use --pre-js, --post-js and another option, let's say ---template-js <file> with a placeholder? Something similar to https://github.com/ni/VireoSDK/blob/master/source/core/vireo.wrapper.js#L30

[kripken]

What's our current status with browserify and webpack - can those use emscripten output now? Or would we need to implement some sort of plugins for them?

Coincidentally I've started to work on a webpack plugin for binaryen, and am learning some npm and webpack as I go...

[curiousdannii]

It's safe if you use MODULARIZE. If you don't then Emscripten will only do a CommonJS export when ENVIRONMENT_IS_NODE is true (#5864). It should be changed to do a full UMD export unconditionally.

[kripken]

I see, thanks @curiousdannii. Yeah, we should definitely document that MODULARIZE is currently required for those.

I wonder if that isn't enough for us? That is, do you think it's worth getting non-MODULARIZE code working with webpack and browserify? Intuitively, I think they expect something like a module and so MODULARIZE being the way to integrate with them seems reasonable?

[curiousdannii]

We definitely should, and it won't be difficult. It's just a matter of moving and expanding these lines:

https://github.com/kripken/emscripten/blob/33afb86a5a4a2ea8cb0bd7c89f8f2af61aff9f27/src/shell.js#L128-L130

to look like these

https://github.com/kripken/emscripten/blob/56e1a9019dd1f3fe9917f0799aa2aa43f7688bad/emcc.py#L2480-L2485

I'll try to get a PR done soon.

In terms of what's expected, non-MODULARIZEd code is fine. After all, MODULARIZE is just adding a wrapping function, which is what Webpack and Browserify do anyway.

Hmm, just had a thought about MODULARIZE_INSTANCE. I haven't used it so I don't know all the rationale for it, but is there a good reason why it doesn't provide .then()? In my mind MODULARIZE_INSTANCE shouldn't really change anything, all it's doing is calling Module() for you, that's all. You can use onRuntimeInitialized in both for example. So why not also provide .then? (It would be possible to provide a then function without MODULARIZE, though I'd prefer we just make it the default instead.)

What would you think about making MODULARIZE be on by default as part of the major release that turns WASM on? IMO it usually makes sense to bundle changing defaults like this.

[kripken]

Well, a downside is that by adding those lines we make the default output larger. I think maybe requiring MODULARIZE be passed to indicate "I want to use this as a module" might make sense. But also maybe I'm overthinking this :)

About MODULARIZE_INSTANCE and .then(), yeah, it seems like we could support that, good idea.

About MODULARIZE on by default, I think that's worth considering, but I actually feel the opposite about bundling stuff - splitting big changes into separate releases lets people upgrade their projects more incrementally, and if something breaks the Changelog can help figure it out more easily with fewer changes per version.

[curiousdannii]

When we add the ES6 modules option that will be much more concise :) Fair enough on your other points.

[stale[bot]]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 7 days. Feel free to re-open at any time if this issue is still relevant.