Fixed deno support. #3990

spigaz · 2024-06-17T22:53:12Z

To be honest I just tried to minimize changes as much as possible to get it working.

daxpedda · 2024-06-18T09:41:02Z

crates/cli/src/bin/wasm-bindgen-test-runner/node.rs

@@ -30,7 +30,7 @@ wrap("info");
 wrap("warn");
 wrap("error");

-cx = new wasm.WasmBindgenTestContext();
+const cx = new wasm.WasmBindgenTestContext();


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

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

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)

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...

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.

daxpedda · 2024-06-18T11:26:04Z

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

@@ -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;",


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.

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.

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.

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.

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.

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.

daxpedda · 2024-06-18T11:27:22Z

@Liamolucko is this something you are able to review?

spigaz · 2024-07-02T11:18:08Z

Thanks to the multi target tests I was able to isolate an issue with the deno handling of the arguments, I already updated it to this PR also.

Liamolucko

LGTM, thanks!

Could you add a changelog entry?

I would've asked you to test this in CI, but it seems like that's already covered by #3920 anyway.

Liamolucko · 2024-07-03T07:12:00Z

crates/cli/src/bin/wasm-bindgen-test-runner/node.rs

@@ -30,7 +30,7 @@ wrap("info");
 wrap("warn");
 wrap("error");

-cx = new wasm.WasmBindgenTestContext();
+const cx = new wasm.WasmBindgenTestContext();


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.

Liamolucko · 2024-07-03T07:15:21Z

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

@@ -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;",


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.

spigaz · 2024-07-03T12:50:28Z

LGTM, thanks!

np

Could you add a changelog entry?

sure, done

I would've asked you to test this in CI, but it seems like that's already covered by #3920 anyway.

Yes, although I'm still working on improving them, so I tried to avoid conflicts as much as possible.

spigaz · 2024-07-21T20:05:53Z

@daxpedda Is there anything missing for this one to be merged?

daxpedda

As mentioned before, I can't really review this.

But seeing that Liamolucko has already approved it, I will go ahead and merge it after the nits are remedied.

crates/cli/src/bin/wasm-bindgen-test-runner/deno.rs

CHANGELOG.md

…ort to CHANGELOG.

Co-authored-by: daxpedda <daxpedda@gmail.com>

…purposes.

daxpedda reviewed Jun 18, 2024

View reviewed changes

Liamolucko approved these changes Jul 3, 2024

View reviewed changes

spigaz force-pushed the main branch from f1bc61c to 91fa2c4 Compare July 21, 2024 19:56

daxpedda reviewed Jul 21, 2024

View reviewed changes

crates/cli/src/bin/wasm-bindgen-test-runner/deno.rs Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

spigaz and others added 6 commits July 22, 2024 10:03

Fixed deno support.

b119706

wasm-bindgen-test-runner: Fixed deno handling of arguments.

3b17bc9

wasm-bindgen-test-runner: Fixed CHANGELOG for deno fix.

f9acbb2

wasm-bindgen-test-runner: Added information regading fixing Deno supp…

3cd7fb1

…ort to CHANGELOG.

Update CHANGELOG.md

c707619

Co-authored-by: daxpedda <daxpedda@gmail.com>

wasm-bindgen-test-runner: Reintroduced the old whitespace for review …

5a886bf

…purposes.

spigaz force-pushed the main branch from 2f6ffd4 to 5a886bf Compare July 22, 2024 09:03

daxpedda merged commit e8b023c into rustwasm:main Jul 22, 2024
23 checks passed

Systemcluster mentioned this pull request Aug 12, 2024

PoC for inline target #4065

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed deno support. #3990

Fixed deno support. #3990

spigaz commented Jun 17, 2024

daxpedda Jun 18, 2024

spigaz Jun 18, 2024

daxpedda Jun 18, 2024

spigaz Jun 18, 2024

Liamolucko Jul 3, 2024

daxpedda Jun 18, 2024 •

edited

Loading

spigaz Jun 18, 2024

daxpedda Jun 18, 2024 •

edited

Loading

spigaz Jun 18, 2024

Liamolucko Jul 3, 2024

spigaz Jul 3, 2024

daxpedda commented Jun 18, 2024

spigaz commented Jul 2, 2024

Liamolucko left a comment

Liamolucko Jul 3, 2024

Liamolucko Jul 3, 2024

spigaz commented Jul 3, 2024

spigaz commented Jul 21, 2024

daxpedda left a comment

Fixed deno support. #3990

Fixed deno support. #3990

Conversation

spigaz commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxpedda Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxpedda Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxpedda commented Jun 18, 2024

spigaz commented Jul 2, 2024

Liamolucko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spigaz commented Jul 3, 2024

spigaz commented Jul 21, 2024

daxpedda left a comment

Choose a reason for hiding this comment

daxpedda Jun 18, 2024 •

edited

Loading

daxpedda Jun 18, 2024 •

edited

Loading