feat(neon): `JsFunction::bind()` and `Object::prop()` #1056

dherman · 2024-07-08T18:15:07Z

This PR implements JsFunction::bind(), which creates a builder for calling a function using the Try{From,Into}Js traits.

let n = f.bind(&mut cx)
    .args((1, 2, 3))?
    .arg(4)?
    .arg_with(|cx| Ok(cx.number(5)))?
    .apply::<f64>()?;

It also implements Object::prop(), which creates a builder for accessing object properties using the Try{From,Into}Js traits.

let x: f64 = obj
    .prop(&mut cx, "x")
    .get()?;

obj.prop(&mut cx, "y")
    .set(x)?;

let s: String = obj.prop(&mut cx, "toString")
    .bind()?
    .apply()?;

crates/neon/src/object/mod.rs

crates/neon/src/types_impl/function/mod.rs

crates/neon/src/types_impl/mod.rs

…unction using the Try{From,Into}Js traits.

…expressively

…ing BindOptions::apply()

…ng non-functions with .bind()

…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()

crates/neon/src/types_impl/mod.rs

Co-authored-by: K.J. Valencik <[email protected]>

kjvalencik · 2024-09-13T19:21:08Z

crates/neon/src/types_impl/mod.rs

+        }
+    }
+}
+
 impl JsFunction {
    /// Create a [`CallOptions`](function::CallOptions) for calling this function.
    pub fn call_with<'a, C: Context<'a>>(&self, _cx: &C) -> CallOptions<'a> {


Do we want to mark call_with or construct_with deprecated?

kjvalencik · 2024-09-13T19:21:35Z

crates/neon/src/types_impl/mod.rs

@@ -1211,6 +1249,18 @@ impl JsFunction {
    }
 }

+impl JsFunction {
+    pub fn bind<'a, 'cx: 'a>(&self, cx: &'a mut Cx<'cx>) -> BindOptions<'a, 'cx> {


Can we add some docs?

kjvalencik · 2024-09-13T19:22:54Z

crates/neon/src/types_impl/function/mod.rs

+    pub(crate) args: private::ArgsVec<'cx>,
+}
+
+impl<'a, 'cx: 'a> BindOptions<'a, 'cx> {


Are we missing a construct method?

kjvalencik · 2024-09-13T19:34:28Z

crates/neon/src/types_impl/function/mod.rs

+///
+/// **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> {}


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)), } } }

kjvalencik · 2024-09-13T19:40:23Z

crates/neon/src/types_impl/function/mod.rs

+
+    /// 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> {


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() }

kjvalencik · 2024-09-13T19:54:06Z

crates/neon/src/object/mod.rs

+    /// 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> {


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<()>.

kjvalencik · 2024-09-13T19:54:28Z

crates/neon/src/object/mod.rs

+    pub(crate) key: K,
+}
+
+impl<'a, 'cx, K> PropOptions<'a, 'cx, K>


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(..)?

kjvalencik · 2024-09-13T20:07:11Z

crates/neon/src/object/mod.rs

+        key: K,
+    ) -> PropOptions<'a, 'cx, K> {
+        let this: Handle<'_, JsObject> =
+            Handle::new_internal(unsafe { ValueInternal::from_local(cx.env(), self.to_local()) });


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.

kjvalencik · 2024-09-13T20:10:41Z

crates/neon/src/types_impl/extract/with.rs

@@ -48,4 +58,21 @@ where
    }
 }

+impl<'cx, F, O> TryIntoArgumentsInternal<'cx> for With<F, O>


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.

kjvalencik · 2024-09-13T20:14:08Z

crates/neon/src/types_impl/mod.rs

+pub(crate) unsafe fn call_local<'a, 'b, C: Context<'a>, T, AS>(
+    cx: &mut C,
+    callee: raw::Local,
+    this: Handle<'b, T>,


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?

dherman changed the title ~~feat(neon): JsFunction::bind()~~ feat(neon): JsFunction::bind() and Object::prop() Jul 9, 2024

dherman marked this pull request as ready for review July 9, 2024 17:42

dherman requested a review from kjvalencik July 10, 2024 22:18

kjvalencik requested changes Jul 12, 2024

View reviewed changes

kjvalencik reviewed Jul 12, 2024

View reviewed changes

crates/neon/src/types_impl/mod.rs Outdated Show resolved Hide resolved

dherman added 9 commits September 8, 2024 15:48

Implement JsFunction::bind(), which creates a builder for calling a f…

cc1b061

…unction using the Try{From,Into}Js traits.

prettier

f7c05b8

cargo fmt --all

8c03fab

Add PropOptions and Object::prop() for accessing object properties …

33b0c2a

…expressively

api doc formatting

acf0ec5

tweak test name for clarity

3d32a52

Optimization: only do the function type check of the callee when call…

db27665

…ing BindOptions::apply()

rebase: {Bind,Prop}Options take Cx instead of generic C: Context

a3f084d

remove unnecessary sys::fun::call helper, and add test case for calli…

589fabc

…ng non-functions with .bind()

dherman force-pushed the prop-options branch from ca0fce8 to 589fabc Compare September 9, 2024 00:59

dherman added 9 commits September 8, 2024 18:01

prettier

59fa3f4

cargo fmt --all

c4fcf56

style: move long type parameter list to where-clause

7ef24b5

more style nits (where-clauses)

d2c770d

arg_with's closure doesn't need to require a Result, since TryIntoJs …

8d18e19

…is already implemented for NeonResult

remove unnecessary unsafe: use .as_value() instead

b2a34c0

BindOptions::this() takes a TryIntoJs (and is therefore fallible)

94a0c08

- tests for both strict and sloppy mode functions to show it works on primitives

store object in PropOptions as a JsObject so it doesn't need a ty…

45be1f3

…pe parameter other than for the key

avoid the double-type-check of eagerly downcasting to function when c…

bb4f231

…alling .bind(), since it's performed when calling .apply()

kjvalencik reviewed Sep 10, 2024

View reviewed changes

crates/neon/src/types_impl/mod.rs Outdated Show resolved Hide resolved

kjvalencik reviewed Sep 10, 2024

View reviewed changes

crates/neon/src/types_impl/mod.rs Outdated Show resolved Hide resolved

dherman and others added 4 commits September 10, 2024 16:56

style: move the InvalidArg tag check to a match guard

dd1ffdf

Co-authored-by: K.J. Valencik <[email protected]>

argc can be usize now that we're not using the c++ backend

3257f01

BindOptions::args_with() and equivalent BindOptions::args(With(...))

6148d8e

more readable logic for impl_arguments! and impl_into_arguments! macros

50a8832

kjvalencik requested changes Sep 13, 2024

View reviewed changes

dherman mentioned this pull request Sep 19, 2024

feat(neon): Add tokio async runtime support #1055

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(neon): `JsFunction::bind()` and `Object::prop()` #1056

feat(neon): `JsFunction::bind()` and `Object::prop()` #1056

dherman commented Jul 8, 2024 •

edited

Loading

kjvalencik Sep 13, 2024

kjvalencik Sep 13, 2024

kjvalencik Sep 13, 2024

kjvalencik Sep 13, 2024

kjvalencik Sep 13, 2024

kjvalencik Sep 13, 2024

kjvalencik Sep 13, 2024

kjvalencik Sep 13, 2024

kjvalencik Sep 13, 2024

kjvalencik Sep 13, 2024

feat(neon): JsFunction::bind() and Object::prop() #1056

Are you sure you want to change the base?

feat(neon): JsFunction::bind() and Object::prop() #1056

Conversation

dherman commented Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(neon): `JsFunction::bind()` and `Object::prop()` #1056

feat(neon): `JsFunction::bind()` and `Object::prop()` #1056

dherman commented Jul 8, 2024 •

edited

Loading