-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
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 I think I already addressed the original painpoint in #2972 (comment). |
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? |
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"; |
Some considerations for @asimpletune and @daxpedda:
That's correct. Cloudflare Workers only require changes to the (very small) bundler file.
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
However, I'd like to point out that:
This PR is an effort to make the task of deploying Rust+Wasm modules online more user-friendly :) |
Not sure why this justifies us supporting it. Rather this is an issue on their side.
True.
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: 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:
I also want to note here that this problem in general has been a big issue in Instead we should push further towards making |
Thanks for your feedback @daxpedda. I see you're heavily hinting at moving this PR towards the
Afaik, the I'll ask people from Cloudflare to chime in - whether this PR's logic ships in |
That indeed would make sense to me then.
Great, thank you! |
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 |
Is there any updates on this? I'm able to run the tests on firefox with wasm_bindgen_test_configure!(run_in_worker); 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. |
I think both @hamza1311 and I covered how we want to proceed in previous posts. |
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! |
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 That said, if the |
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. |
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 |
I'm going to close this due to inactivity. The request is continuing to be tracked in #3891. |
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 calledcf-worker
.The current
cf_worker.js
binding generated bywasm-bindgen --target bundler
is:This PR, following Cloudflare's own documentation and using
wasm-bindgen --target bundler --workerd
, creates the following binding instead:which is indeed compatible with Cloudflare Workers.