-
Notifications
You must be signed in to change notification settings - Fork 14
Add wasm package #53
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
Add wasm package #53
Conversation
This lets us compile the lib into wasm and adds a npm wrapper package for it I had to rearrange the code slightly to split out any methods using wasm incompatible types. Not sure if there's a better way to do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds WebAssembly support for the library along with an npm wrapper package.
- Adds documentation for building and publishing the npm/wasm package.
- Introduces tests for the WASM package in the JS wrapper.
- Applies wasm-bindgen annotations and adjusts Cargo.toml to support WASM builds.
Reviewed Changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/string-offsets/CONTRIBUTING.md | Adds instructions for building, testing, and publishing the WASM/JS package. |
| crates/string-offsets/js/test/test.js | Introduces sanity-check tests for the WASM package. |
| crates/string-offsets/src/lib.rs | Applies wasm-bindgen attributes and reorganizes method implementations. |
| crates/string-offsets/Cargo.toml | Configures the library for CDYLIB builds and adds the wasm-bindgen dependency. |
Files not reviewed (2)
- crates/string-offsets/.gitignore: Language not supported
- crates/string-offsets/js/package.json: Language not supported
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
tclem
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool, looks great to me. Having to move some methods to another impl block seems fine.
(Maybe as a follow up, we should add this to the Actions CI workflow)
tclem
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This lets us compile the lib into wasm and adds a npm wrapper package for it
I had to rearrange the code slightly to split out any methods using wasm incompatible types. Not sure if there's a better way to do this