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

add deno target #2176

Merged
merged 5 commits into from Jun 3, 2020
Merged

add deno target #2176

merged 5 commits into from Jun 3, 2020

Conversation

jakobhellermann
Copy link
Contributor

adds a new OutputMode for Deno.

Exports and js-imports are generated like OutputMode::Web or OutputMode::Node { experimental_modules: true }.

wasm-files cannot be imported directly, so the file is read at runtime. If embedding the file as a base64 string or similar is preferable, I can try to do that.

Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Dealing with various kinds of import systems can be pretty finnicky and tough to maintain over time. Would it be possible to add a test for this on CI?

crates/cli-support/src/js/mod.rs Outdated Show resolved Hide resolved
crates/cli-support/src/js/mod.rs Show resolved Hide resolved
@jakobhellermann
Copy link
Contributor Author

looks like the test finally passes.
I copied the import_js example, because that way imports and exports are being tested, and modified it a little bit to include passing a string to a function.
If anything else should be tested, I can add that.

Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I was also wondering, as well, would it be possible to run the full test suite in deno? For example cargo test --test wasm should in theory work for the wasm target. I don't think it exercises any node-specific behavior.

Additionally can you update guide/src/reference/deployment.md with the new deno option?

crates/cli-support/src/js/mod.rs Outdated Show resolved Hide resolved
@jakobhellermann
Copy link
Contributor Author

I'm not sure what would need to be done to run the test suit in deno.
Add a new file similar to crates/cli/src/bin/wasm-bindgen-test-runner/node.rs for deno and add wasm_bindgen_test_configure!(deno)?

@alexcrichton
Copy link
Contributor

Something like that would work, yeah, although since it's so similar to node it might be best to control with an env var rather than in the code itself. Either way though it's not the most urgent thing in the world. Running the full test suite would provide a lot of confidence that the wrapper generation here is working for all use cases, but since this is a new feature it doesn't have to do everything right out of the gate.

@jakobhellermann
Copy link
Contributor Author

alright, I tried running the test suite in deno but there seem to be two problems:

  1. assert_eq!(export_name, definition_name) fails with Color != Color2. This is because Color is defined in tests/wasm/enum.rs and tests/wasm/node.rs.
    I'm not sure why this isn't asserted for node and what would be the right thing to do here.
  2. All the #[wasm_bindgen(module = ...)] attributes only work in node because of some hack with NODE_PATH. I'm not sure what the best approach would be towards removing that hack. Copying every imported file into wbg-tmp?

@jakobhellermann
Copy link
Contributor Author

also all the tests use exports.foo = ... and require.

Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok I think it's fine in that case to go ahead and land this in the meantime. We can perhaps track in an issue what it might look like to run in Deno on CI? Ideally we'd just switch everything to ES imports instead of using the weird hacks that are there today.

It also looks like a wasm file was checked in here too?

examples/deno/crate/Cargo.toml Outdated Show resolved Hide resolved
guide/src/reference/deployment.md Outdated Show resolved Hide resolved
@alexcrichton
Copy link
Contributor

All looks great to me, thanks!

@alexcrichton alexcrichton merged commit 9c5a6df into rustwasm:master Jun 3, 2020
@jakobhellermann jakobhellermann deleted the deno-target branch June 3, 2020 21:55
@Pauan
Copy link
Contributor

Pauan commented Jun 4, 2020

I know this has already been merged, but why does --target web not work?

Why can't you just do await init(Deno.readFileSync("path/to/foo_bg.wasm"));?

@jakobhellermann
Copy link
Contributor Author

To be honest I didn't quite understand why the init function was needed and thought there were some changes to be done anyway because for Deno the file wouldn't be fetched but Deno.readFileed instead. But I just tried await init(Deno.readFileSync("path/to/foo_bg.wasm")); and it works perfectly...

@jakobhellermann
Copy link
Contributor Author

maybe wasm-pack can instead have a deno target using --target web which generates a mod.ts with

// @deno-types=foo.d.ts
export * from "./foo.js";
import init from "./foo.js";
init(Deno.readFileSync(import.meta.url logic));

@Pauan
Copy link
Contributor

Pauan commented Jun 4, 2020

@jakobhellermann The init function accepts string, Request, Url, ArrayBuffer, Uint8Array, or WebAssembly.Module.

If you pass in an ArrayBuffer, Uint8Array or WebAssembly.Module then it will not fetch. This allows you to handle all of the loading yourself (such as with Deno).

maybe wasm-pack can instead have a deno target using --target web which generates a mod.ts

That won't work, because then you cannot access the exports of the Rust code:

import init, * as exports from "./foo.js";

init(Deno.readFileSync("./foo_bg.wasm")).then(() => {
    // Use exports...
});

Instead, I recommend using our Rollup plugin, which will automatically call init for you. You can customize the loading behavior like this:

rust({
    importHook: function (path) {
        return "Deno.readFileSync(" + JSON.stringify(path) + ")";
    },
})

@alexcrichton
Copy link
Contributor

I think it might still makes sense to have a deno target for "it works similarly to node" where it auto-inits, but perhaps the generated could could be refactored to use top-level await like @Pauan mentions so it's basically --target web but with one extra line?

@Pauan
Copy link
Contributor

Pauan commented Jun 4, 2020

@alexcrichton I'm okay with that, though I'm a bit concerned that it might create a precedent for other targets.

@alexcrichton
Copy link
Contributor

It's true yeah I don't want to create a proliferation of too many targets, and @Pauan if you'd like I think it's fine if we revert this and continue discussion on an issue (sorry should have consulted first!)

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

Successfully merging this pull request may close these issues.

None yet

3 participants