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

[quickjs-wasm-rs] Should the closure passed to wrap_callback return JSValueRef? #608

Closed
surma opened this issue Feb 27, 2024 · 3 comments
Closed
Labels
enhancement New feature or request

Comments

@surma
Copy link
Collaborator

surma commented Feb 27, 2024

Currently, the closure passed to wrap_callback is impl FnMut(&Self, JSValueRef<'_>, &[JSValueRef<'_>]) -> Result<JSValue> + 'static.

This has worked well so far, however, this makes it impossible to return an already existing value and maintain referential equality. For example:

let ctx = JSContextRef::default();
// This is supposed to be a Rust-implementation of `x => x`.
let identity_func = ctx.wrap_callback(|ctx, _this, args| {
    let undef = ctx.undefined_value()?;
    // As far as I can tell, `from_qjs_value` is the only path from `JSValueRef` 
    // to `JSValue`, which deep-copies the value into a context-independent data structure.
    from_qjs_value(args.get(0).cloned().unwrap_or(undef))
})?;
let object = ctx.object_value()?;
let result = identity_func.call(&ctx.undefined_value()?, &[func.clone()])?;
// Will fail as these are different objects now.
assert_eq!(Into::<u64>::into(result), Into::<u64>::into(object));

As JSValue is a context-independent representation of a JS value, would it make sense for a callback to always return a context-specific value, which can be easily created from a given JSValue using to_qjs_value()?

(Side-note: Would be great to also implement PartialEq for JSValueRef.)

@surma surma added the enhancement New feature or request label Feb 27, 2024
@saulecabrera
Copy link
Member

I think we did it in this way initially assuming that all use-cases would want to return high-level Rust types from the callback, but taking a close look what you're suggesting, it doesn't seem like the right assumption and makes sense to me also because wrap_callback immediately converts the JSValue to a JSValueRef in which case if you want to pass an existing JSValueRef the process feels somewhat inefficient. I think that we could make it so that it works for both use cases, and let the caller decide what return type they are after depending on the use-case.

@jeffcharles
Copy link
Collaborator

Another example of where it would be useful to be able to return a JSValueRef would be if someone wants to return another function from their function. See this Zulip chat message for where this has come up.

@saulecabrera saulecabrera mentioned this issue Mar 19, 2024
5 tasks
@saulecabrera
Copy link
Member

Now that #618 is merged, this is no longer an issue. I'll go ahead and close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants