Skip to content
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

Option to specify extra javascript imports required when instantiating WASM Module #1584

Open
jrandall opened this issue Jun 10, 2019 · 4 comments

Comments

@jrandall
Copy link

Motivation

The init function generated by wasm-bindgen adds an import bindings it generates, but there does not currently appear to be any way to specify additional imports that the WASM module may require. My immediate use case is to be able to provide a 'wasi_unstable': WASIPolyfill import in order to support a WASI library to which my wasm-bindgen project links, but this general facility could be used whenever a WASM module needs direct access to javascript in the browser that doesn't go through wasm-bindgen (i.e. it should make it easier for wasm-bindgen to interoperate with other mechanisms for binding between WASM and javascript).

Proposed Solution

I am not sure where is the best place would be to implement this option. The import needs to run before the wasm module is loaded, so it seems like it ought to be pure javascript, so it doesn't really make sense to annotate any rust code with the #[wasm_bindgen] macro to configure it.

For this reason it seems like the best option might be to have a crate level setting that just defines all extra imports as a javascript fragment that just gets dumped into the generated init function. However, there don't appear to be any other aspects of #[wasm_bindgen] that work that way, so I'm not sure exactly what to suggest.

Alternatives

Despite it being perhaps a bit awkward, perhaps It could be configured from the #[wasm_bindgen] macro using a self-contained annotation that would functions similar to an inline_js that indicates that it generates an import binding (can the macro annotate an empty code block or something like that?).

Perhaps something like:

#[wasm_bindgen(init_import_js = "function(imports) {imports.wasi_unstable = WASIPolyfill}")]

or

#[wasm_bindgen(init_import_js = "function() {return {"wasi_unstable": WASIPolyfill}}")]
@Pauan
Copy link
Contributor

Pauan commented Jun 10, 2019

To clarify, I believe you are talking about the import object which is passed to WebAssembly.instantiate.

So you want something like this:

WebAssembly.instantiate(buffer, {
    "./rustc_h_gnju0uqy2i9": __exports,
    "wasi_unstable": WASIPolyfill
})

Correct?

@alexcrichton
Copy link
Contributor

Thanks for the report @jrandall! Assuming @Pauan's interepretation is correct (which also matches my reading here) I'm not certain how the best way to handle this would be.

I like the idea of specifying something in the Rust source code in that presumably when you're importing something you know what you want to import, and when someone imports you they may not know what you wanted to import. For something like wasi_unstable though it's somewhat tricky because there's multiple implementations and an implementation could come from a few locations.

An alternative possibility which goes against putting it in the source would be something like:

import init from './the.js';

init({wasi_unstable: WASIPolyfill});

(or something like that where it's passed to the init function)

I think though we'd probably want to shoot for something that ideally works in all output modes.

It may be the case though that wasi_unstable is really the only case here? That may mean that we could just special case that in wasm-bindgen and avoid needing to create a special import mechanism for now since it's pretty rare to have imports in Rust code that aren't already matched up to something else

@jrandall
Copy link
Author

jrandall commented Jul 30, 2019

Sorry to have not noticed earlier updates and questions on this issue until now. The above interpretation by @Pauan is correct, and yes, wasi_unstable is the only use case I currently have, so having built-in support for that would address my needs. I can see how more generic support for adding dependencies to the module load could be cumbersome, so limiting this to specific support for WASI would work for me. The only issue I see with that is that presumably at some point the interface will change from wasi_unstable to wasi (or perhaps a versioned namespace?) at which point we would need a way to specify what the namespace should be.

I would note that I do have this all working using patch as a workaround (so I can confirm that the rest of the story of wasm-bindgen working with a WASI module works without issue). However, I am limited to building without webpack or modules (currently using --target no-modules) and I am not certain how I would get webpack working without fixing this centrally. Getting this working using the --target bundler deployment is the main reason why I'd like to see support for this, so ideally the fix would work with all output modes.

The patch file I apply to the js bindings built by wasm-bindgen is:

--- dist/pkg/client.js       2019-06-06 11:01:39.040269283 +0000
+++ wasi/pkg/client.js       2019-06-06 11:07:59.923201883 +0000
@@ -437,7 +437,7 @@

 function init(module) {
     let result;
-    const imports = { './client': __exports };
+    const imports = { './client': __exports, 'wasi_unstable': WASIPolyfill };

     if (module instanceof URL || typeof module === 'string' || module instanceof Request) {

@@ -467,6 +467,7 @@
         });
     }
     return result.then(({instance, module}) => {
+        setInstance(instance);
         wasm = instance.exports;
         init.__wbindgen_wasm_module = module;
         wasm.__wbindgen_start();

The latter part of the patch is a workaround to the other issue (#1576) required for WASI support, which is that we need to be able to get the instance out of the init function so that we can call WASI setInstance() on it. If we were to limit this issue to generic support for WASI in wasm-bindgen, then it would make sense to have a single fix that enables WASI support within the init function and includes a call to setInstance(instance) (as in the above patch) as well as setting the WASI implementation in the import.

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue Jul 31, 2019
Support was previously (re-)added in rustwasm#1654 for importing direct JS
values into a WebAssembly module by completely skipping JS shim
generation. This commit takes that PR one step further by *also*
embedding a direct import in the wasm file, where supported. The wasm
file currently largely just imports from the JS shim file that we
generate, but this allows it to directly improt from ES modules where
supported and where possible. Note that like rustwasm#1654 this only happens
when the function signature doesn't actually require any conversions to
happen in JS (such as handling closures).

For imports from ES modules, local snippets, or inline JS they'll all
have their import directives directly embedded into the final
WebAssembly binary without any shims necessary to hook it all up. For
imports from the global namespace or possibly vendor-prefixed items
these still unconditionally require an import shim to be generated
because there's no way to describe that import in an ES-friendly way
(yet).

There's a few consequences of this commit which are also worth noting:

* The logic in `wasm-bindgen` where it gracefully handles (to some
  degree) not-defined items now only is guaranteed to be applied to the
  global namespace. If you import from a module, it'll be an
  instantiation time error rather than today's runtime error when the
  import is called.

* Handling imports in the wasm module not registered with
  `#[wasm_bindgen]` has become more strict. Previously these imports
  were basically ignored, leaving them up for interpretation depending
  on the output format. The changes for each output target are:

  * `bundler` - not much has changed here. Previously these ignored
    imports would have been treated as ES module imports, and after this
    commit there might just be some more of these imports for bundlers
    to resolve.

  * `web` - previously the ignored imports would likely cause
    instantiation failures because the import object never actually
    included a binding for other imports. After this commit though the
    JS glue which instantiates the module now interprets all
    unrecognized wasm module imports as ES module imports, emitting an
    `import` directive. This matches what we want for the direct import
    functionality, and is also largely what we want for modules in
    general.

  * `nodejs` - previously ignored imports were handled in the
    translation shim for Node to generate `require` statements, so they
    were actually "correctly handled" sort of with module imports. The
    handling of this hasn't changed, and reflects what we want for
    direct imports of values where loading a wasm module in Node ends up
    translating the module field of each import to a `require`.

  * `no-modules` - this is very similar to the `web` target where
    previously this didn't really work one way or the other because we'd
    never fill in more fields of the import object when instantiating
    the module. After this PR though this is a hard-error to have
    unrecognized imports from `#[wasm_bindgen]` with the `no-modules`
    output type, because we don't know how to handle the imports.

  Note that this touches on rustwasm#1584 and will likely break the current use
  case being mentioned there. I think though that this tightening up of
  how we handle imports is what we'll want in the long run where
  everything is interpreted as modules, and we'll need to figure out
  best how wasi fits into this.

This commit is unlikely to have any real major immediate effects. The
goal here is to continue to inch us towards a world where there's less
and less JS glue necessary and `wasm-bindgen` is just a polyfill for web
standards that otherwise all already exist.

Also note that there's no explicitly added tests for this since this is
largely just a refactoring of an internal implementation detail of
`wasm-bindgen`, but the main `wasm` test suite has many instances of
this path being taken, for example having imports like:

    (import "tests/wasm/duplicates_a.js" "foo" (func $__wbg_foo_969c253238f136f0 (type 1)))
    (import "tests/wasm/duplicates_b.js" "foo" (func $__wbg_foo_027958cb2e320a94 (type 0)))
    (import "./snippets/wasm-bindgen-3dff2bc911f0a20c/inline0.js" "trivial" (func $__wbg_trivial_75e27c84882af23b (type 1)))
    (import "./snippets/wasm-bindgen-3dff2bc911f0a20c/inline0.js" "incoming_bool" (func $__wbg_incomingbool_0f2d9f55f73a256f (type 0)))
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue Jul 31, 2019
Support was previously (re-)added in rustwasm#1654 for importing direct JS
values into a WebAssembly module by completely skipping JS shim
generation. This commit takes that PR one step further by *also*
embedding a direct import in the wasm file, where supported. The wasm
file currently largely just imports from the JS shim file that we
generate, but this allows it to directly improt from ES modules where
supported and where possible. Note that like rustwasm#1654 this only happens
when the function signature doesn't actually require any conversions to
happen in JS (such as handling closures).

For imports from ES modules, local snippets, or inline JS they'll all
have their import directives directly embedded into the final
WebAssembly binary without any shims necessary to hook it all up. For
imports from the global namespace or possibly vendor-prefixed items
these still unconditionally require an import shim to be generated
because there's no way to describe that import in an ES-friendly way
(yet).

There's a few consequences of this commit which are also worth noting:

* The logic in `wasm-bindgen` where it gracefully handles (to some
  degree) not-defined items now only is guaranteed to be applied to the
  global namespace. If you import from a module, it'll be an
  instantiation time error rather than today's runtime error when the
  import is called.

* Handling imports in the wasm module not registered with
  `#[wasm_bindgen]` has become more strict. Previously these imports
  were basically ignored, leaving them up for interpretation depending
  on the output format. The changes for each output target are:

  * `bundler` - not much has changed here. Previously these ignored
    imports would have been treated as ES module imports, and after this
    commit there might just be some more of these imports for bundlers
    to resolve.

  * `web` - previously the ignored imports would likely cause
    instantiation failures because the import object never actually
    included a binding for other imports. After this commit though the
    JS glue which instantiates the module now interprets all
    unrecognized wasm module imports as ES module imports, emitting an
    `import` directive. This matches what we want for the direct import
    functionality, and is also largely what we want for modules in
    general.

  * `nodejs` - previously ignored imports were handled in the
    translation shim for Node to generate `require` statements, so they
    were actually "correctly handled" sort of with module imports. The
    handling of this hasn't changed, and reflects what we want for
    direct imports of values where loading a wasm module in Node ends up
    translating the module field of each import to a `require`.

  * `no-modules` - this is very similar to the `web` target where
    previously this didn't really work one way or the other because we'd
    never fill in more fields of the import object when instantiating
    the module. After this PR though this is a hard-error to have
    unrecognized imports from `#[wasm_bindgen]` with the `no-modules`
    output type, because we don't know how to handle the imports.

  Note that this touches on rustwasm#1584 and will likely break the current use
  case being mentioned there. I think though that this tightening up of
  how we handle imports is what we'll want in the long run where
  everything is interpreted as modules, and we'll need to figure out
  best how wasi fits into this.

This commit is unlikely to have any real major immediate effects. The
goal here is to continue to inch us towards a world where there's less
and less JS glue necessary and `wasm-bindgen` is just a polyfill for web
standards that otherwise all already exist.

Also note that there's no explicitly added tests for this since this is
largely just a refactoring of an internal implementation detail of
`wasm-bindgen`, but the main `wasm` test suite has many instances of
this path being taken, for example having imports like:

    (import "tests/wasm/duplicates_a.js" "foo" (func $__wbg_foo_969c253238f136f0 (type 1)))
    (import "tests/wasm/duplicates_b.js" "foo" (func $__wbg_foo_027958cb2e320a94 (type 0)))
    (import "./snippets/wasm-bindgen-3dff2bc911f0a20c/inline0.js" "trivial" (func $__wbg_trivial_75e27c84882af23b (type 1)))
    (import "./snippets/wasm-bindgen-3dff2bc911f0a20c/inline0.js" "incoming_bool" (func $__wbg_incomingbool_0f2d9f55f73a256f (type 0)))
@alexcrichton
Copy link
Contributor

FWIW I've posted #1689 which tightens up some of the handling of unknown import directives to wasm-bindgen (including wasi ones). I think for now this will need to continue be handled with patch (although perhaps in a slightly different way) or with bundler mappings in theory. As the name impiles the wasi module story is still in development, so I'm not sure we'll want to set too much in stone just yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants