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

Add escape hatches to and from quickjs_wasm_sys #610

Merged
merged 6 commits into from
Feb 29, 2024
Merged

Conversation

surma
Copy link
Collaborator

@surma surma commented Feb 27, 2024

While quickjs_wasm_rs provides a much more ergonomic API, there are some features of QuickJS that are not exposed. For example, exotic object behaviors or QuickJS’s execution timeout.

In the long-run, it would be great to have those in quickjs_wasm_rs, but in the meantime, it seems like a good idea regardless to me expose escape hatches to the low-level crate.

Specifically:

  • Expose the wasm_quick_sys module from quickjs_wasm_rs to avoid users having to add that crate themselves and potentially ending up with different version.
  • Add a function to convert raw QuickJS value (u64) into a JSValueRef
  • Add functions to convert a *mut JSContext to a JSContextRef and vice versa (in fact, *mut JSContext was already exposed via new_callback, but hard to utilize without the other parts).

Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

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

For example, exotic object behaviors or QuickJS’s execution timeout.

Could you add a bit more detail about which APIs you'd like to see exposed? I'm not very familiar with "exotic object behaviours" for example.

Generally my stance is that quickjs_wasm_rs should be as safe as it can be; I think this change is not changing that fundamental principle given that the safety around the methods is correctly annotated and we're not enforcing the usage of unsafe code, it's pretty much opt-in.

I'd be good though to have a list of features that you think we're missing in quickjs_wasm_rs to be able to get get rid of these "escape hatches", so I'd appreciate if you could list those more concretely if you can.

@surma
Copy link
Collaborator Author

surma commented Feb 27, 2024

I'd be good though to have a list of features that you think we're missing in quickjs_wasm_rs to be able to get get rid of these "escape hatches", so I'd appreciate if you could list those more concretely if you can.

I mean the short answer is: All QuickJS features 😅

Specifically for my explorations right now, I want to make use of the following QuickJS features:

  • JS_SetInterruptHandler which calls a callback in regular intervals that can choose to terminate the execution (by returning true).
  • Exotic Methods, which are a part of a QuickJS class definition. It effectively allows you to implement a JS class whose shape and implementation is backed by Rust. I’m currently working around this by using the ES6 Proxy API, but that is a level of indirection I’d like to avoid in the future :)

In general I think these escape hatches are valuable and I don’t think they really pose a risk, but it’s totally your call.

@saulecabrera
Copy link
Member

saulecabrera commented Feb 27, 2024

In general I think these escape hatches are valuable and I don’t think they really pose a risk, but it’s totally your call.

I think they are too IMO, and we should land this, given that we currently don't cover the entire surface area of the QuickJS API. But in the future, ideally we should support all the features in a safe way, hence my comment about not making any stability guarantees. And as mentioned above, these new methods are are not forcing anyone to use any unsafe operations.

Exotic Methods, which are a part of a QuickJS class definition. It effectively allows you to implement a JS class whose shape and implementation is backed by Rust

Oh ok, that makes sense. This is probably going to be useful for #605

@jeffcharles
Copy link
Collaborator

Would there be any value in putting the re-export of quickjs-wasm-sys and the escape hatches behind a cargo feature? That might help communicate the stability guarantees in code as well as in documentation.

@surma
Copy link
Collaborator Author

surma commented Feb 27, 2024

@jeffcharles I don’t think I have seen that be done before, but I like the suggestion.

@surma surma requested a review from saulecabrera February 28, 2024 08:40
@saulecabrera
Copy link
Member

saulecabrera commented Feb 28, 2024

It seems like one of the bindings tests is failing, I think it could be related to the fix in as_str, because all the other changes seem unrelated.

@jeffcharles
Copy link
Collaborator

Please update the CHANGELOG file for quickjs-wasm-rs and update the version of the crate to 3.1.0-alpha.1 since this adds meaningful new features.

@surma surma force-pushed the surma/escapehatch branch from eccef4e to 6ae8a32 Compare February 29, 2024 09:35
@surma
Copy link
Collaborator Author

surma commented Feb 29, 2024

@jeffcharles @saulecabrera I think this is good to go

@saulecabrera saulecabrera merged commit 3d3e07f into main Feb 29, 2024
14 checks passed
@saulecabrera saulecabrera deleted the surma/escapehatch branch February 29, 2024 11:54
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.

3 participants