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

Feat: add support for Cloudflare Workers via --workerd option #3510

Closed
wants to merge 4 commits into from

Conversation

jkomyno
Copy link

@jkomyno jkomyno commented Jul 6, 2023

Currently, the bindings generated by wasm-bindgen cannot be used on Cloudflare Workers out of the box (see "JavaScript Plumbing" section in the docs). This is a pain for developers, as shown in #2972.

This PR proposes a new --workerd option for --target bundler that closes #2972.

Tests and markdown documentation updates are included.

Example

Consider a wasm32-unknown-unknown crate called cf-worker.
The current cf_worker.js binding generated by wasm-bindgen --target bundler is:

import * as wasm from "./cf_worker_bg.wasm";
import { __wbg_set_wasm } from "./cf_worker_bg.js";
__wbg_set_wasm(wasm);
export * from "./cf_worker_bg.js";

This PR, following Cloudflare's own documentation and using wasm-bindgen --target bundler --workerd, creates the following binding instead:

import * as imports from "./cf_worker_bg.js";
import wkmod from "./cf_worker_bg.wasm";
const instance = new WebAssembly.Instance(wkmod, { "./cf_worker_bg.js": imports });
imports.__wbg_set_wasm(instance.exports);
export * from "./cf_worker_bg.js";

which is indeed compatible with Cloudflare Workers.

@daxpedda
Copy link
Collaborator

daxpedda commented Jul 7, 2023

Unfortunately I know nothing about bundlers, so I will need some help understanding this.

Considering this PR doesn't actually touch the JS shim, only the final bundler file (the JS file without _bg), can't users not add this on their own before using the bundler? It seems straightforward to me, it's basically like users having to add the index.html file when using the web target.

I think I already addressed the original painpoint in #2972 (comment).

@daxpedda daxpedda self-assigned this Jul 7, 2023
@daxpedda daxpedda added the needs discussion Requires further discussion label Jul 7, 2023
@asimpletune
Copy link

I guess one question I have is what would accomplishing the same thing look like from the user's perspective without this PR? Like, what steps would they have to take?

@daxpedda
Copy link
Collaborator

daxpedda commented Jul 7, 2023

Like, what steps would they have to take?

Following the proposed solution of this PR, the only step the user would have to take is replace the output file with this:

import * as imports from "./cf_worker_bg.js";
import wkmod from "./cf_worker_bg.wasm";
const instance = new WebAssembly.Instance(wkmod, { "./cf_worker_bg.js": imports });
imports.__wbg_set_wasm(instance.exports);
export * from "./cf_worker_bg.js";

@jkomyno
Copy link
Author

jkomyno commented Jul 7, 2023

Some considerations for @asimpletune and @daxpedda:

this PR doesn't actually touch the JS shim

That's correct. Cloudflare Workers only require changes to the (very small) bundler file.

the only step the user would have to take is replace the output file with this

import * as imports from "./cf_worker_bg.js";
import wkmod from "./cf_worker_bg.wasm";
const instance = new WebAssembly.Instance(wkmod, { "./cf_worker_bg.js": imports });
imports.__wbg_set_wasm(instance.exports);
export * from "./cf_worker_bg.js";

Essentially, yes, keeping in mind that cf_worker is an example name and not a constant string.
Let's call the file above bundler-template.js and refer to the variable shim filename (without the postfix _bg.js part) as $WASM_SHIM_NAME. In this example, WASM_SHIM_NAME=cf_worker.

what would accomplishing the same thing look like from the user's perspective without this PR?

  1. run wasm-bingen --target bundler
  2. in the output folder, replace $WASM_SHIM_NAME.js's content with bundler-template.js' content
  3. run a :s/cf_worker/$WASM_SHIM_NAME/g search&replace command on $WASM_SHIM_NAME.js

However, I'd like to point out that:

  • Cloudflare Workers explicit refer to wasm-bindgen in their documentation
  • Cloudflare Workers are one of the major platforms for deploying Wasm modules online in a serverless fashion
  • I think Cloudflare Workers are big enough to justify a first-class support (which, as you can see, doesn't require major changes in wasm-bindgen's source code) without having to resort to third-party utils / Wasm binding patch scripts.

This PR is an effort to make the task of deploying Rust+Wasm modules online more user-friendly :)

@daxpedda
Copy link
Collaborator

daxpedda commented Jul 7, 2023

  • Cloudflare Workers explicit refer to wasm-bindgen in their documentation

Not sure why this justifies us supporting it. Rather this is an issue on their side.

  • Cloudflare Workers are one of the major platforms for deploying Wasm modules online in a serverless fashion

True.

  • I think Cloudflare Workers are big enough to justify a first-class support (which, as you can see, doesn't require major changes in wasm-bindgen's source code) without having to resort to third-party utils / Wasm binding patch scripts.

I don't disagree, but on the other hand it would be better if they provide support when they are doing non-standard stuff.

Case in point: worker-build. From personal experience I know that Cloudflare itself doesn't maintain it very well, but it seems better to me to let them support it as long as we provide the tools necessary to do so.

I think it's gonna be problematic if the JS shim needs adjustment, then I would lean in favor of providing better support, but if it's only the bundle file, I think some effort from Cloudflare's side is appropriate.

I'm currently against merging this PR:

  • The web target already works as intended.
  • The bundle target can be used by modifying/fixing the bundler file.
  • A dedicated tool for Cloudflare Workers already exists: worker-build. So this should be out of our hands to begin with, aside from making sure we continue to support such tools.

I also want to note here that this problem in general has been a big issue in wasm-bindgen. The Web is pretty diverse and has a lot of very different tools. wasm-bindgen already supports a bunch of tools I personally have no clue about and it's already unmaintainable as-is, widening the scope even further is not really a good idea imo.

Instead we should push further towards making wasm-bindgen as agnostic as possible so other tools can be built on top of it to support all these diverse needs. worker-build is a perfect example on how we should handle this imho.

@jkomyno
Copy link
Author

jkomyno commented Jul 7, 2023

Thanks for your feedback @daxpedda. I see you're heavily hinting at moving this PR towards the worker-build tool. To shed additional context on this - Cloudflare Workers support WebAssembly in two different scenarios:

  1. When defining workers in JavaScript/TypeScript, you can import Wasm modules (which, referencing this PR, requires wasm-bindgen --target bundler --workerd)
  2. When defining workers purely in Rust via workers-rs, to compile to Wasm them via worker-build and upload them.

Afaik, the worker-build tool is only useful in this latter case. This PR addresses the first case.

I'll ask people from Cloudflare to chime in - whether this PR's logic ships in wasm-bindgen or in Cloudflare's own infrastructure is rather indifferent to me, as long as all Cloudflare Workers devs can avoid an additional "glue code patching" step (as it's currently the case).

@daxpedda
Copy link
Collaborator

daxpedda commented Jul 7, 2023

  1. When defining workers in JavaScript/TypeScript, you can import Wasm modules (which, referencing this PR, requires wasm-bindgen --target bundler --workerd)

That indeed would make sense to me then.

I'll ask people from Cloudflare to chime in - whether this PR's logic ships in wasm-bindgen or in Cloudflare's own infrastructure is rather indifferent to me, as long as all Cloudflare Workers devs can avoid an additional "glue code patching" step (as it's currently the case).

Great, thank you!

@hamza1311
Copy link
Collaborator

hamza1311 commented Jul 7, 2023

I'm not a fan of this. What about environments other than workerd? Cloudflare isn't the only one providing a worker environment where wasm-bindgen generated code will run. Would we be supporting every environment with its own flag, as needed? I don't think that's a good idea

Fixing #2972 isn't a bad thing. But I don't think this is the way to do this. One solution for #2972 could be to let init function take the WebAssembly.Module

@spigaz
Copy link

spigaz commented Nov 30, 2023

Is there any updates on this?

I'm able to run the tests on firefox with wasm_bindgen_test_configure!(run_in_worker);
or wasm_bindgen_test_configure!(run_in_browser);

But I wanted to run at least some of them on the workerd, because of the workerd extra stuff, like D1.

Is there a way for the runner to load them on workerd?

I believe that one way or another, this goes through either forking your runner or improving your runner to handle it.

@daxpedda
Copy link
Collaborator

I think both @hamza1311 and I covered how we want to proceed in previous posts.
If anything is unclear let us know.

@spigaz
Copy link

spigaz commented Nov 30, 2023

Sorry @daxpedda, I have read several times your comments, and they only talk about producing final files ready to be deployed to workerd.

And to be honest I understand perfectly your reasoning.

But using wasm-bindgen produced test files to run the tests with your test runner using workerd is a bit of a different use case.

The only thing that I can infer from them, regarding my question, is that your point of view is that CF should also provide their own test runner.

Like cargo wasix or webassembly_test that target wasmtime.

I understand that, that's okay.

I'm willing to pitch in, so its relevant for me to know what makes sense to invest my time in.

Thank you!

@daxpedda
Copy link
Collaborator

daxpedda commented Nov 30, 2023

Apologies, you are right your case is different, I didn't pay enough attention.

I think the same philosophy applies to the test runner as well. The preference here it not to have to cover every use case there is in wasm-bindgen. So as you said, CF should provide their own test runner.

That said, if the wasm-bindgen test runner can be adjusted to provide a general way for other runtimes/platforms to inject what they need to get things to work, that would be great as well.
That would obviously require some design work, but considering the lack of time from maintainers this has to be done in small steps that can be easily discussed and reviewed.

@spigaz
Copy link

spigaz commented Dec 1, 2023

I'm already in contact with someone from CF to see how this can be done, his comment was that my approach seemed reasonable.

But at the moment it seems they don't export enough yet to run the tests, but this is something that I do require, so I'm going to start experimenting soon anyway.

I'll update you when I have relevant info.

@hamza1311
Copy link
Collaborator

using wasm-bindgen produced test files to run the tests with your test runner using workerd is a bit of a different use case.

That is a use case I would love to support. There's also a point to be made of running wasm-bindgen output in non-node.js environments like Deno or Bun. I think this PR is out of scope for this discussion. Can you create a new issue for this so we can discuss how to handle this? I think we can take inspiration from the Jasmine testing framework in the JS ecosystem

@daxpedda
Copy link
Collaborator

I'm going to close this due to inactivity.

The request is continuing to be tracked in #3891.

@daxpedda daxpedda closed this Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Requires further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS glue: Please provide a way to initialize with an already loaded WebAssembly.Module
5 participants