Skip to content

Conversation

@mjbvz
Copy link
Contributor

@mjbvz mjbvz commented Mar 13, 2025

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

mjbvz added 3 commits March 13, 2025 11:21
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
Copilot AI review requested due to automatic review settings March 13, 2025 18:26
@mjbvz mjbvz requested a review from a team as a code owner March 13, 2025 18:26
Copy link
Contributor

Copilot AI left a 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

Copy link
Member

@tclem tclem left a 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)

Copy link
Member

@tclem tclem left a comment

Choose a reason for hiding this comment

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

LGTM

@tclem tclem merged commit 0524b48 into github:main Mar 18, 2025
3 checks passed
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.

4 participants