-
Notifications
You must be signed in to change notification settings - Fork 283
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
feat(neon): JsFunction::bind()
and Object::prop()
#1056
base: main
Are you sure you want to change the base?
Conversation
JsFunction::bind()
JsFunction::bind()
and Object::prop()
…unction using the Try{From,Into}Js traits.
…ing BindOptions::apply()
…ng non-functions with .bind()
ca0fce8
to
589fabc
Compare
…is already implemented for NeonResult
- tests for both strict and sloppy mode functions to show it works on primitives
…pe parameter other than for the key
…alling .bind(), since it's performed when calling .apply()
Co-authored-by: K.J. Valencik <kjvalencik@gmail.com>
} | ||
} | ||
} | ||
|
||
impl JsFunction { | ||
/// Create a [`CallOptions`](function::CallOptions) for calling this function. | ||
pub fn call_with<'a, C: Context<'a>>(&self, _cx: &C) -> CallOptions<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to mark call_with
or construct_with
deprecated?
@@ -1211,6 +1249,18 @@ impl JsFunction { | |||
} | |||
} | |||
|
|||
impl JsFunction { | |||
pub fn bind<'a, 'cx: 'a>(&self, cx: &'a mut Cx<'cx>) -> BindOptions<'a, 'cx> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some docs?
pub(crate) args: private::ArgsVec<'cx>, | ||
} | ||
|
||
impl<'a, 'cx: 'a> BindOptions<'a, 'cx> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we missing a construct
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to rename .apply()
to .call()
so we have .call()
and .construct()
.
/// | ||
/// **Note:** This trait is implemented for tuples of up to 32 JavaScript values, | ||
/// but for the sake of brevity, only tuples up to size 8 are shown in this documentation. | ||
pub trait TryIntoArguments<'cx>: private::TryIntoArgumentsInternal<'cx> {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add an implementation for Result<T, E>
? Currently .args_with(..)
requires the closure to be infallible.
If we add this implementation than you can return (1, 2, 3)
or Ok((1, 2, 3))
.
impl<'cx, T, E> TryIntoArguments<'cx> for Result<T, E>
where
T: TryIntoArguments<'cx>,
E: TryIntoJs<'cx>,
{
}
impl<'cx, T, E> private::TryIntoArgumentsInternal<'cx> for Result<T, E>
where
T: private::TryIntoArgumentsInternal<'cx>,
E: TryIntoJs<'cx>,
{
fn try_into_args_vec(self, cx: &mut Cx<'cx>) -> NeonResult<private::ArgsVec<'cx>> {
match self {
Ok(v) => v.try_into_args_vec(cx),
Err(err) => err.try_into_js(cx).and_then(|err| cx.throw(err)),
}
}
}
|
||
/// Make the function call. If the function returns without throwing, the result value | ||
/// is converted to a Rust value with `TryFromJs::from_js`. | ||
pub fn apply<R: TryFromJs<'cx>>(&mut self) -> NeonResult<R> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add an fn exec(&mut self) -> NeonResult<()>
for no return type? It's functionally equivalent to .apply::<()>()
since ()
has an infallible TryFromJs
implementation, but it seems like a nice convenience to avoid the turbofish.
pub fn exec(&mut self) -> NeonResult<()> {
self.apply()
}
/// Equivalent to calling `obj.set(cx, v.try_into_js(cx)?)`. | ||
/// | ||
/// May throw an exception either during converting the value or setting the property. | ||
pub fn set<V: TryIntoJs<'cx>>(&mut self, v: V) -> NeonResult<bool> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we eliminate the bool
? That has some odd historical context with the NAN backend and is meaningless now (AFAICT, it will always be true
). Something useful might be NeonResult<&mut Self>
because you could chain operations.
We could also add PropOptions::prop
method that changes the key. It could make for a nice object builder API.
let o = cx.empty_object()
.prop("a")
.set("this is a")?
.prop("b")
.set("this is b")?
.to_value();
The chaining is up to you, but at the least we should change the return to NeonResult<()>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without chaining:
let o = cx.empty_object();
o.prop("a").set("this is a")?;
o.prop("b").set("this is b")?;
o.prop("c").set("this is c")?;
pub(crate) key: K, | ||
} | ||
|
||
impl<'a, 'cx, K> PropOptions<'a, 'cx, K> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be useful to add a to_value()
helper to get this
back out?
Additionally, can we add a set_with<V: TryIntoJs<'cx>(..)
for similar reasons to arg_with(..)
?
key: K, | ||
) -> PropOptions<'a, 'cx, K> { | ||
let this: Handle<'_, JsObject> = | ||
Handle::new_internal(unsafe { ValueInternal::from_local(cx.env(), self.to_local()) }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are extending the lifetime 'self
to 'cx
. If I understand correctly, this is safe because 'cx
will always be the shortest possible lifetime of all handles. This is because any longer contexts will be "locked" with a &mut self
.
In other words, this is safe because Neon invariants enforce 'self: 'cx
for all handles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should add this to a // SAFETY
comment in the code.
@@ -48,4 +58,21 @@ where | |||
} | |||
} | |||
|
|||
impl<'cx, F, O> TryIntoArgumentsInternal<'cx> for With<F, O> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, it makes a bit more sense to me for this implementation to be with the TryIntoArguments
code rather than here, but if you think it's better here, I'm okay with that.
pub(crate) unsafe fn call_local<'a, 'b, C: Context<'a>, T, AS>( | ||
cx: &mut C, | ||
callee: raw::Local, | ||
this: Handle<'b, T>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, it's a little odd for this to take a Handle
when callee
is a raw::Local
. Can these both be handles or both Local
?
This PR implements
JsFunction::bind()
, which creates a builder for calling a function using theTry{From,Into}Js
traits.It also implements
Object::prop()
, which creates a builder for accessing object properties using theTry{From,Into}Js
traits.