Switch to rquickjs
#618
Merged
Switch to rquickjs
#618
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Migrated the `Console` and `Random` APIs; got to the `StreamIO` API.
3 tasks
This commit migrates the `console`, `stream_io`, `random` and `text_encoding` APIs to use rquickjs. One notable change in this commit is the introduction of the `Args` struct to tie the lifetime of `Ctx<'js>` and `Rest<Value<'js>>` arguments, given that explicit lifetime binding is not possible in Rust closures at the moment.
saulecabrera
force-pushed
the
javy-rquickjs
branch
from
aa9a9a8
to
878ddd5
Compare
|
Not quite ready for review, but almost. I want to see what CI says. |
surma
reviewed
Apr 11, 2024
saulecabrera
force-pushed
the
javy-rquickjs
branch
from
a918d3b
to
85abf57
Compare
|
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. |
…nder the javy crate
`cargo` doesn't seem to pickup the fact that `quickcheck` is only used in tests.
And prune unused ones
saulecabrera
requested review from
surma,
makrisoft and
jeffcharles
and removed request for
makrisoft
saulecabrera
commented
Apr 16, 2024
saulecabrera
commented
Apr 16, 2024
I wanted to avoid as much as possible using the underlying QuickJS unsafe APIs, but that introduced behavioral changes and bugs. So for now, I'm sticking to the previous implementation.
jeffcharles
reviewed
Apr 16, 2024
I'll do that as a follow-up. I added an entry in the PR description. |
jeffcharles
approved these changes
Apr 17, 2024
|
Oops meant to push 5fde19c, into a copy branch to do some testing -- I'll revert it. |
saulecabrera
force-pushed
the
javy-rquickjs
branch
from
5fde19c
to
29361f3
Compare
makrisoft
approved these changes
Apr 23, 2024
This commit introduces rquickjs 0.6.0 through a fork that contains Wasm specific performance improvements. We intend to upstream these improvements.
saulecabrera
commented
May 1, 2024
Adds special policy for rquickjs given that we're currently in a fork.
saulecabrera
commented
May 1, 2024
jeffcharles
reviewed
May 1, 2024
This was referenced May 2, 2024
Closed
saulecabrera
added a commit
to saulecabrera/javy
that referenced
this pull request
May 6, 2024
This commit is a follow-up to bytecodealliance#618, to formally mark `quckjs-wasm-{sys, rs}` crates as deprecated. The rollout strategy is to: * Merge this PR * Publish the a new version of each of the crates, containing the last unreleased change in each and the deprecation notice.
This was referenced May 6, 2024
saulecabrera
added a commit
that referenced
this pull request
May 7, 2024
This commit is a follow-up to #618, to formally mark `quckjs-wasm-{sys, rs}` crates as deprecated. The rollout strategy is to: * Merge this PR * Publish the a new version of each of the crates, containing the last unreleased change in each and the deprecation notice.
|
Thanks a lot for doing rquickjs switch work .Till when can I expect the new release on github roughly ? |
|
Mid June is my estimate: we're doing more extensive testing and working on other improvements before releasing the next version. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
rquickjsand dropping support forquickjs-wasm-rsandquickjs-wasm-sys. Therquickjsbindings 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: #605, #608, #617 and #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-rsis 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 theMIN_SAFE_INTEGER..=MAX_SAFE_INTEGERrange.This change is already big as-is; I'm planning on creating follow-up PRs for:
docsquickjs-wasm-rsandquickjs-wasm-syswith a deprecation notice.Checklist
javy-cliandjavy-coredo not require updating CHANGELOG files.