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: import .wasm modules in service worker format workers #432

Merged
merged 1 commit into from
Feb 11, 2022

Conversation

threepointone
Copy link
Contributor

This allows importing .wasm modules in service worker format workers. We do this by hijacking imports to .wasm modules, and instead registering them under [wasm_modules] (building on the work from #409).

@changeset-bot
Copy link

changeset-bot bot commented Feb 10, 2022

🦋 Changeset detected

Latest commit: e681b66

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2022

wrangler prerelease is available for testing:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/1829576326/wrangler

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

I believe that index_bg.js is identical between the three Workers.
Perhaps we could just move that next to the index_bg.wasm file to remove duplication?

The wasm file generated, `index_bg.wasm` is copied into `./worker`, and shared by the 2 workers. `./worker/module` contains a "modules" format worker and imports the wasm module as a regular es module, while `./worker/service-worker` contains a "service-worker" format worker and uses wrangler.toml to bind the wasm module as a global `MYWASM`. They're otherwise identical.
The wasm file generated, `index_bg.wasm` is copied into `./worker`, and shared by the 3 workers.

- `./worker/module` contains a "modules" format worker and imports the wasm module as a regular es module.
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Every where I look it is ES module not es module... 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I replaced it across the codebase.

Comment on lines +68 to +76
contents: `export default ${args.path.replace(
/[^a-zA-Z0-9_$]/g,
"_"
)};`,
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment explaining the point of this code would be helpful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some comments. Do they look ok to you?

This allows importing `.wasm` modules in service worker format workers. We do this by hijacking imports to `.wasm` modules, and instead registering them under `[wasm_modules]` (building on the work from #409).
@threepointone
Copy link
Contributor Author

threepointone commented Feb 11, 2022

I missed the bit about index_bg.js. The problem there is that the module it imports "./export_wasm.js" is different across them, so I don't want to refactor that, not much to gain.

@threepointone
Copy link
Contributor Author

Actually, lemme land this. We can change the comments whenever.

@threepointone threepointone merged commit 78acd24 into main Feb 11, 2022
@threepointone threepointone deleted the wasm-modules branch February 11, 2022 13:54
@github-actions github-actions bot mentioned this pull request Feb 11, 2022
mrbbot added a commit that referenced this pull request Oct 31, 2023
…ts (#432)

* Allow `string[]` for `kvNamespaces`/`r2Buckets` like Miniflare 2

In this case, the binding name is assumed to be the same as the
namespace ID/bucket name.

* Add API documentation
mrbbot added a commit that referenced this pull request Nov 1, 2023
…ts (#432)

* Allow `string[]` for `kvNamespaces`/`r2Buckets` like Miniflare 2

In this case, the binding name is assumed to be the same as the
namespace ID/bucket name.

* Add API documentation
mrbbot added a commit that referenced this pull request Nov 1, 2023
…ts (#432)

* Allow `string[]` for `kvNamespaces`/`r2Buckets` like Miniflare 2

In this case, the binding name is assumed to be the same as the
namespace ID/bucket name.

* Add API documentation
mrbbot added a commit that referenced this pull request Nov 1, 2023
…ts (#432)

* Allow `string[]` for `kvNamespaces`/`r2Buckets` like Miniflare 2

In this case, the binding name is assumed to be the same as the
namespace ID/bucket name.

* Add API documentation
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.

2 participants