search

bytecodealliance / javy

JS to WebAssembly toolchain
Apache License 2.0
2.1k stars 101 forks source link

Switch to `rquickjs` #618

Closed saulecabrera closed 3 months ago

saulecabrera commented 4 months ago

Description of the change

Historically, the Javy project has relied on its own QuickJS bindings in order to consume QuickJS' functionality. When the project started, there were very limited options for bindings that could work for our use-case and more importantly, bindings that would compile to wasm32-wasi. That said, bindings under this project are "good enough" for Javy, but they are probably not the most ergonomic for use-cases that require more complex interactions with QuickJS.

The Rust <> QuickJS landscape has changed in the past few years, which means that it's probably a good time to reconsider our initial decisions.

This PR proposes migrating to rquickjs and dropping support for quickjs-wasm-rs and quickjs-wasm-sys. The rquickjs bindings are more ergonomic and enable more natural interactions between Rust and QuickJS' API.

With the adoption of rquickjs, we'd be able to address issues like: https://github.com/bytecodealliance/javy/issues/605, https://github.com/bytecodealliance/javy/issues/608, https://github.com/bytecodealliance/javy/pull/617 and https://github.com/bytecodealliance/javy/pull/611.

This change is divided in a series of commits, but it's better reviewed as a whole, given that I didn't put enough thought to make each commit self-contained.

In general, this change doesn't introduce any functional changes, dropping support for quickjs-wasm-rs is the only breaking change introduced by this PR. The only exception is that I fixed a bug in our number deserialization process, in which the implementation was not respecting the MIN_SAFE_INTEGER..=MAX_SAFE_INTEGER range.

This change is already big as-is; I'm planning on creating follow-up PRs for:

Checklist

saulecabrera commented 3 months ago

Not quite ready for review, but almost. I want to see what CI says.

saulecabrera commented 3 months ago

Ok, so all tests are passing here. I'm working on migrating our json/msgpack ser/de and that should be the last step (aside from vetting the new dependencies) to get this in a review-ready state.

saulecabrera commented 3 months ago

We should update the READMEs for quickjs-wasm-sys and quickjs-wasm-rs to indicate they are deprecated in favour of rquickjs. Would that make sense to do as part of this PR or in a follow up one?

I'll do that as a follow-up. I added an entry in the PR description.

saulecabrera commented 3 months ago

Oops meant to push https://github.com/bytecodealliance/javy/pull/618/commits/5fde19c7316d693bb895e0b169a038622a83e657, into a copy branch to do some testing -- I'll revert it.

redoC-A2k commented 2 months ago

Thanks a lot for doing rquickjs switch work .Till when can I expect the new release on github roughly ?

saulecabrera commented 2 months ago

Mid June is my estimate: we're doing more extensive testing and working on other improvements before releasing the next version.