-
Notifications
You must be signed in to change notification settings - Fork 786
Build Binaryen.js to an ES module #3304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
var __dirname = "."; |
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.
We either may derive this from import.meta
using a mechanism like node's fileURLToPath
, or look into enabling import.meta
otherwise, either using EXPORT_ES6
or something new.
Types[entry[0]] = value; | ||
Types[value] = entry[0]; |
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.
Unrelated: Adds a reverse lookup to the enums, as TypeScript typically does and expects when typed as enum
in TS definitions.
wrapModule(MODULE['_BinaryenModuleCreate'](), this); | ||
} | ||
|
||
export { BynModule as Module }; |
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.
A little unfortunate, but not a problem. Module
is already defined, so we can only export as
to expose the proper name.
// Default export one gets upon either | ||
// * `import binaryen from "binaryen"` or | ||
// * `const binaryen = require("binaryen")`. | ||
export default { |
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.
The repeated code here is somewhat typical for hybrid modules, in that it ensures that the module works ergonomically the same when require
d. When transpiled to CommonJS, this becomes module.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.
Most (all?) of the functions here seem to be duplications of inline export
statements. Why do you need them both as inline exports and as default object? Ideally you'd only keep the former, and transpilers can handle their conversion to CommonJS fine. It's also preferable, because named exports are tree-shakeable by other tooling, whereas with default object you lose this benefit of ESM.
One didn't work without the other anyway - before that PR it was just throwing an error rather than auto-enabling the required option. I'm not sure I understand what you want and how it's different from |
Oh, you're doing all the exports manually? Hmm. Instead of doing pre/post on the original module, wouldn't it be easier to create a separate wrapper that will |
What Binaryen.js is doing is essentially what
Aren't we getting two different files then, exporting one from importing the other? |
Yeah, but in most cases it's not a problem, and then if you use something like Rollup for CommonJS build, it will just correctly inline one into another anyway. |
This particular thing should be possible independently of |
Yep, that's the option I was looking at as well 👍 I guess what we could do is to add an |
Ah, and of course, to not break existing use cases, we can keep |
I think Binaryen is quite special in this regard because it uses custom pre/post JS with exports. I don't see what the semantics would be for normal Emscripten output under It needs Modularize because it can only export a factory where user could providing a path to the Wasm file as well as instantiation options, and not because it tries to allow multiple instantiations (it's more of a side effect). For Node.js these concerns might not apply, since it has a real filesystem API, but whatever Emscripten generates should work in all environments. |
I really think that the best option in this case is, instead of custom pre/post, to let Emscripten generate regular This would be a path of the least resistance, and has much lower chance of breaking whenever we regenerate the "internal" JS+Wasm. |
It only uses the custom extern-pre.js an extern-post.js because Binaryen's use case is not sufficiently covered in Emscripten anymore since the removal of |
Thinking about this a little more, perhaps the mechanism |
I think you might be putting too much focus on this possibility (instantiating a module multiple times), even though it's not the goal of Instead, In this regard The fact that it exports a factory is only a side-effect of having to accept a config from outside before instantiation, and not a design choice to encourage you to create several instances. It's okay to still instantiate it only once in a singleton and re-export only what you need. |
Yeah, MODULARIZE solves some problems, but it also introduces new ones for ESM specifically by mandating the factory function etc. For instance, the only way to emit |
This is maybe a stupid question: can an ES6 module be imported ("instantiated") more than once? If it can't, then it seems like ES6 mode should emit something like what |
No, it can't.
I'm not sure that's true / how would that work. User does need a factory function at least to 1) be able to pass in params before any instantiation occurs and 2) to get a Promise back. You can alleviate (2) by exporting a promise as a variable, or by waiting till top-level await is supported in various engines, but I don't see how you would work around (1) because that's the part user does need to think about and be able to config. Also, making |
Interesting, thanks @RReverser How do normal JS modules deal with those two problems (initialization and async start)? |
Two ways: like Emscripten already does, by exposing a factory, or by exporting a separate function to init the module. In my personal experience, the first one is more common and more intuitive, because such API design ensures that user doesn't attempt to access other exports before initialization, whereas second approach (used by, for example, wasm-bindgen output) makes it too easy to make such mistakes for the user and attempt to access exported variables / functions when the underlying state is not yet initialised. As another alternative, we could add another mode / compile-time option that would assume that the user doesn't want to configure the To summarise, I think that, in the current state of things, a factory export like the one Emscripten already uses, is the least error-prone solution as it can be used in any of the given scenarios - whether it's non-configurable init, init with a custom config, single instance or multiple instances of the module. |
This is an experiment on what will be necessary to build Binaryen.js as an ES module (see #2990 (comment)), in particular allowing usage like
In general appears to be possible, except that Emscripten automatically enables
MODULARIZE
whenEXPORT_ES6
is given (see emscripten-core/emscripten#12530, cc @RReverser), which is not what we want here, yet we wantEXPORT_ES6
(or a new option) to enableexport
syntax in Emscripten's acorn optimizer which otherwise fails with an error, and usage ofimport.meta
instead of__dirname
. Hence, for now, only debug builds build.