rustwasm / wasm-bindgen

Facilitating high-level interactions between Wasm modules and JavaScript
https://rustwasm.github.io/docs/wasm-bindgen/
Apache License 2.0
7.82k stars 1.08k forks source link

Cannot import an ES6 module after JS snippets change #1343

Closed BigBigos closed 5 years ago

BigBigos commented 5 years ago

The b762948456617ee263de8e43b3636bd3a4d1da75 commit has overloaded the meaning of #[wasm_bindgen(module = "...")]. Previously, it has defined an ES6 module import that the JS shim performed and provided to the wasm module. Now it also allows the contents of a JS file to be embedded as a snippet.

The new behavior is problematic when the copied module has imports on its own - the relative paths lose their meaning. I would like to still be able to perform an import to the original path without copying the module.

Unanchored paths (i.e. ones that don't start with either /, . or ..) still behave the same as before. However, I need to use a relative path starting with ../, which is explicitly disallowed.

Pauan commented 5 years ago

Relative paths within a .js file are intended to work correctly, it just hasn't been implemented yet.

@alexcrichton Maybe we should back out the changes until it is feature-complete?

alexcrichton commented 5 years ago

@BigBigos and @ZacLiveEarth could y'all comment a bit on the uses cases you had which were working before? While it was possible to get module working with relative paths I would imagine that it was pretty wonky and fragile to set up, so I'm curious how you managed to plumb it through.

@Pauan I unfortunately don't actually know how to implement relative paths given the current set of stable proc-macro apis, because otherwise I'd be more than willing to implement them!

ZacLiveEarth commented 5 years ago

@alexcrichton Sure. The module worked with a relative path the first try, I wasn't aware of any issues or complexities surrounding it. Basically, we had an existing React app that we wanted to add Rust to, and we added the rust modules for it in a subdirectory of the main react app. By necessity then to create bindings to functions in the original React app we needed a relative path to escape from the Rust subdirectory. This just works in 0.2.28

alexcrichton commented 5 years ago

@ZacLiveEarth but the relative path isn't actually relative to the Rust code, right? Rather it's relative to the final output wasm file? (just confirming)

Also can you expand a bit on the build process? Are you using a process like webpack to pull in the wasm module into the main app? Additionally is this JS file referenced here included via anything else or is it only referenced by the wasm file?

BigBigos commented 5 years ago

@alexcrichton In my case, I am creating a web app that is based on the wasm module written in Rust. The wasm module (alongside the wasm-bindgen bindings) is output to the /pkg directory. I have a separate /js directory with some JS code, which I want to reference from the Rust code.

Previously I was using #[wasm_bindgen(module = "../js/module")] in order to import a JS module from Rust. After the snippet change, I had to change the path to "/js/module.js" which was still fine. However, I wanted to import the wasm ES6 module itself from a JS module in order to access the wasm memory. I put import * as wasm from { "../pkg/cratename_bg" } inside the JS module, which would work if it stayed in /js. However, as the snippets are copied to /pkg/snippets/... this relative path no longer works.

I have worked-around this by adding a webpack alias js -> /js. This way I can do #[wasm_bindgen(module = "js/module")] and it works.

In case it matters, the Rust code is in /src, i.e. the Cargo.toml is in the main / directory.

alexcrichton commented 5 years ago

Ok thanks for the info @BigBigos!

One thing I forgot to mention earlier is that the change here is a product of RFC 6 for those interested to read over as well.

FWIW the breakage here is "somewhat intentional" that we wanted to tighten the restrictions on module and crack down a bit on cases that "happened to work" previously but weren't intended to work long term. That being said though it is not intentional that the upgrade to 0.2.39 is a large hurdle for anyone and if it is then I we may consider backing out and tweaking.

@BigBigos for importing the wasm file itself that was discussed in RFC 6 so you may be interested in reading up on that!

ZacLiveEarth commented 5 years ago

For completeness, here's the full code.

type GLEnum = u32;

#[wasm_bindgen(module = "../../../../src/modules/wasm-binds.js")]
extern "C" {
    #[wasm_bindgen(js_name = createVertexBuffer)]
    fn create_vertex_buffer_f32(typed_array: Float32Array, usage: GLEnum) -> JsValue;
}

It seems that it was relative to the output wasm file (in the pkg directory) by sheer luck. Since we only did this once so far, we never had a chance to figure that out.

Our build process uses webpack. I'm not intimately familiar with the details of that and the guy who set it up is out of office.

BigBigos commented 5 years ago

@BigBigos for importing the wasm file itself that was discussed in RFC 6 so you may be interested in reading up on that!

I didn't know that, thanks! This will certainly simplify a bit of my code and make the snippet mode work for this use case.

The other limitation of imports between JS snippet modules still remains. I will just keep using my work-around described above until it is implemented. Or do you intend to remove support for #[wasm_bindgen(module = "...")] with unanchored paths some time in the future? I understand that they have similar issues as relative paths, but in my case I can manage the paths (I don't intend to publish the wasm module on its own).

ZacLiveEarth commented 5 years ago

I started reading RFC 6 and am very concerned about the section on depending on additional JS files.

Being able to depend on other JS files (and indeed, over a thousand of them not including npm modules) is very important to us.

The strategy that seems to be implied by the RFC is that an additional 'copy' of the module and it's imports will be embedded in the wasm. I may be misunderstanding the implications of this, but if it means what I think that's a non-starter.

Here's some of the problems that I see with that:

  1. Increased delivery size - megabytes in our case.
  2. No propagation of 'module state'. Many of our modules contain private variables which maintain global state that is setup by the javascript as a pre-condition of calling the wasm.

Javascript build processes and their implications are not my strong suit, so please let me know if my concerns are misplaced here.

alexcrichton commented 5 years ago

@BigBigos oh module is definitely here to stay, the purpose of the local snippets was to repurpose the meaning of relative paths to try to ensure that crates published to crates.io can use local JS snippets as well.

@ZacLiveEarth oh JS files aren't copied into the wasm module, but rather snippets are all copied to an appropriate location in the output directory so a bundler can work with them. One of the main goals of RFC 6 was to enable crates published to crates.io to be able to write and depend on JS snippets.

For the end-user use case (which I think @BigBigos and @ZacLiveEarth y'all both fall into) the RFC isn't really helping much as you already had this working somewhat! The main target was library authors (like rand, for example) which want to include local JS snippets into the final product.

I wonder though we can perhaps solve this with a new key like #[wasm_bindgen(module = "./foo.js", no_include_snippet)]? That would be an explicit way to opt out of "this is a snippet which needs processing"

Pauan commented 5 years ago

@alexcrichton Bit of bike shedding: maybe it should be #[wasm_bindgen(raw_module = "./foo.js")] instead?

That could also be useful for importing raw npm modules (like fs, etc.)

BigBigos commented 5 years ago

I wonder though we can perhaps solve this with a new key like #[wasm_bindgen(module = "./foo.js", no_include_snippet)]? That would be an explicit way to opt out of "this is a snippet which needs processing"

Something like that would work for me. I like the @Pauan raw_module suggestion more than the no_include_snippet, but naming things is not my strong point.

ZacLiveEarth commented 5 years ago

I like @Pauan 's direction for the name better than no_include_snippet as well. I might use the word 'import' instead of 'raw_module' though, since that carries with it all the normal connotations of javascript imports.

It does seem like snippets and imports are orthogonal concepts, which may both be important for different use cases.

In any case, the pre-existing behavior of 0.2.28 was perfect for our use case, sans confusion about the base path for the relative import. If we rename that functionality to import or raw_module or add a no_snippets flag or whatever, that works for me as well. I think you understand the use case well enough that I'll leave you to the remaining value judgements here as you understand the particular implementation details of wasm-bindgen and wasm-pack better.

Pauan commented 5 years ago

Tiny bit more bikeshedding:

I don't have any particular preference, I think they're all fine.

alexcrichton commented 5 years ago

I'm personally a fan of raw_module because then we don't have to deal with #[wasm_bindgen(raw)] and rationalizing that somehow. If there's no objection to that I can look to implement this soon!

ZacLiveEarth commented 5 years ago

No objection.

alexcrichton commented 5 years ago

Ok! I've pushed up https://github.com/rustwasm/wasm-bindgen/pull/1353 with these changes.