Skip to content

Fixed deno support. #3990

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jul 22, 2024
Merged

Fixed deno support. #3990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b119706
Fixed deno support.
spigaz Jun 17, 2024
3b17bc9
wasm-bindgen-test-runner: Fixed deno handling of arguments.
spigaz Jul 2, 2024
f9acbb2
wasm-bindgen-test-runner: Fixed CHANGELOG for deno fix.
spigaz Jul 21, 2024
3cd7fb1
wasm-bindgen-test-runner: Added information regading fixing Deno supp…
spigaz Jul 3, 2024
c707619
Update CHANGELOG.md
spigaz Jul 21, 2024
5a886bf
wasm-bindgen-test-runner: Reintroduced the old whitespace for review …
spigaz Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@

* Fix MDN links to static interface methods.
[#4010](https://github.com/rustwasm/wasm-bindgen/pull/4010)

* Fixed Deno support.
[#3990](https://github.com/rustwasm/wasm-bindgen/pull/3990)

--------------------------------------------------------------------------------

Expand Down
3 changes: 2 additions & 1 deletion crates/cli-support/src/js/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,8 @@ impl<'a> Context<'a> {
}}

const wasmInstance = (await WebAssembly.instantiate(wasmCode, imports)).instance;
const wasm = wasmInstance.exports;",
const wasm = wasmInstance.exports;
export const __wasm = wasm;",
Copy link
Collaborator

@daxpedda daxpedda Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Note for reviewer: this is probably undesirable and at the very least should be limited to Deno.

EDIT: This part of the code already limits it to Deno.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't broke node or any of the implementations I tested so far, so I wasn't sure if deno was just being more strict than the others.

Copy link
Collaborator

@daxpedda daxpedda Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm more worried about exporting anything, even if prefixed by __, unless its absolutely necessary. We don't want anyone starting to rely on it unless we intend to and we also don't want to occupy a name unless necessary.

Again, there is no real issue here, we should just double-check if this is really necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your concerns, the generated code requires this variable both on node and on deno

const ok = await cx.run(tests.map(n => wasm.__wasm[n]))

For some reason, without the export it says its not declared on deno, perhaps because deno uses an import:

import * as wasm from "./{0}.js";

while node still uses a require:

const wasm = require("./{0}")

In practice it was being "exported" for node already, intentionally or not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like this was added for Node only in #2012:

wasm-bindgen/crates/cli-support/src/js/mod.rs

Line 278 in ad82651

module.exports.__wasm = wasm;

I don't like it very much, but this isn't making things any worse and removing it from Node is probably out of scope for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about the best approach either here or in other places, but it seems clear to me that some refactors are in order.

I don't think its just me, but when I'm just trying to fix issues, I try to avoid refactoring as much as possible to make the PR easier to review, but that strategy adds up, and its pretty clear that some refactors are in order.

Although, in this case and some others, I'm not sure the best approach is, as my point of view was quite narrow, focused on specific issues.

But I think opening issues with those changes, so that someone can pick up and provide a PR might be a very good idea.

module_name = module_name
)
}
Expand Down
12 changes: 7 additions & 5 deletions crates/cli/src/bin/wasm-bindgen-test-runner/deno.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,13 @@ pub fn execute(

{console_override}

// global.__wbg_test_invoke = f => f();
window.__wbg_test_invoke = f => f();

// Forward runtime arguments. These arguments are also arguments to the
// `wasm-bindgen-test-runner` which forwards them to deno which we
// forward to the test harness. this is basically only used for test
// filters for now.
cx.args(Deno.args.slice(1));

const ok = await cx.run(tests.map(n => wasm.__wasm[n]));
if (!ok) Deno.exit(1);
cx.args(Deno.args);

const tests = [];
"#,
Expand All @@ -39,6 +36,11 @@ pub fn execute(
js_to_execute.push_str(&format!("tests.push('{}')\n", test));
}

js_to_execute.push_str(
r#"const ok = await cx.run(tests.map(n => wasm.__wasm[n]));
if (!ok) Deno.exit(1);"#,
);

let js_path = tmpdir.join("run.js");
fs::write(&js_path, js_to_execute).context("failed to write JS file")?;

Expand Down
2 changes: 1 addition & 1 deletion crates/cli/src/bin/wasm-bindgen-test-runner/headless.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use anyhow::{bail, format_err, Context, Error};
use log::{debug, warn};
use rouille::url::Url;
use serde::{Deserialize, Serialize};
use serde_json::{self, json, Map, Value as Json};
use serde_json::{json, Map, Value as Json};
use std::env;
use std::fs::File;
use std::io::{self, Read};
Expand Down
7 changes: 5 additions & 2 deletions crates/cli/src/bin/wasm-bindgen-test-runner/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ wrap("info");
wrap("warn");
wrap("error");

cx = new wasm.WasmBindgenTestContext();
const cx = new wasm.WasmBindgenTestContext();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this is an unrelated change?
No need to remove it, just noting for other reviewers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that is required for deno, because deno implementation uses a part of the node implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be good if you explain what you did exactly and why you did it in the OP or comments.

(maybe its more obvious for the actual reviewer)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest I don't remember all the details.

But by looking at the diffs, I don't think I made any decisions, I tried to follow the original intentions, I just added missing pieces and changed the order of some code to get it working.

I agree its hard to understand because the errors surfaced mainly in the generated run.js file, in this case, I was getting a variable missing error.

On another the variable was used before it was declared for instance...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the old version didn't work in Deno because it has strict mode enabled by default, which doesn't allow implicitly declaring global variables like that.

handlers.on_console_debug = wasm.__wbgtest_console_debug;
handlers.on_console_log = wasm.__wbgtest_console_log;
handlers.on_console_info = wasm.__wbgtest_console_info;
Expand Down Expand Up @@ -117,7 +117,10 @@ pub fn execute(
#[cfg(unix)]
pub fn exec(cmd: &mut Command) -> Result<(), Error> {
use std::os::unix::prelude::*;
Err(Error::from(cmd.exec()).context("failed to execute `node`"))
Err(Error::from(cmd.exec()).context(format!(
"failed to execute `{}`",
cmd.get_program().to_string_lossy()
)))
}

#[cfg(windows)]
Expand Down
10 changes: 9 additions & 1 deletion crates/test/src/rt/detect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ extern "C" {
#[wasm_bindgen(method, getter, structural)]
fn constructor(me: &Scope) -> Constructor;

#[wasm_bindgen(method, getter, structural, js_name = Deno)]
fn deno(me: &Scope) -> Option<Deno>;

type Deno;

type Constructor;
#[wasm_bindgen(method, getter, structural)]
fn name(me: &Constructor) -> String;
Expand All @@ -27,7 +32,10 @@ pub fn detect() -> Runtime {
"DedicatedWorkerGlobalScope"
| "SharedWorkerGlobalScope"
| "ServiceWorkerGlobalScope" => Runtime::Worker,
_ => Runtime::Browser,
_ => match scope.deno() {
Some(_) => Runtime::Node,
_ => Runtime::Browser,
},
},
None => Runtime::Node,
}
Expand Down