Arguments builder API

[dherman]

First Second N'th (I lost count) draft of a function arguments builder API, which tries to provide a more ergonomic experience for invoking JS functions.

Benefits

• Arguments don't need to be explicitly wrapped in a vector
• No need to store this binding in a temp variable before invoking the function
• Default this binding of undefined for common case
• Heterogeneous arguments without explicit upcasting
• Overloaded result type so callers don't need to explicitly downcast

Before:

let this = cx.undefined();
let args = vec![
    cx.number(17).upcast(),
    cx.string("hello").upcast()
];
let v: Handle<JsArray> = f.call(&mut cx, this, args)?
    .downcast_or_throw()?;

After:

let v: Handle<JsArray> = f
    .call_with(&cx)
    .arg(cx.number(17))
    .arg(cx.string("hello"))
    .apply(&mut cx)?;

Open questions

• Since .call() is generic in two types (the context and the result type), turbofish instantiation is a little ugly (e.g. .call::<_, JsArray>(&mut cx)). You can usually do the instantiation in a let binding instead (see the above example), and .exec() avoids having to instantiate in the case where you're ignoring the result. So maybe this is OK, but I'm just calling it out.
• I made Arguments implement Clone so you can reuse and call an instance of the builder multiple times. We could possibly make call() etc not consume self but would that require forcing an extra vector copy?
• Are we OK with requiring .args(()) for the empty arguments case? It seems like the least-annoying option we've come up with, compared to:
  • f.with().call(&mut cx)
  • f.args::<JsValue>().call(&mut cx)
  • The one other option I can think of is backwards-incompatibly removing the existing .call() method from JsFunction and having .call(), .new(), and .exec() methods directly on JsFunction. But this could be quite a disruptive change to our existing users.

[kjvalencik]

I really like this API. This is a big improvement. I think it can be optimized further if we don't tie ourselves to using the existing function call implementation and instead go directly to Node-API.

Goals

• Avoid allocating multiple vecs
• Avoid iterating multiple times
• Non-consuming builder

Since both Handle and JsValue are #[repr(C)] with only a single field, these types are equivalent to napi_value.

It should be safe to transmute directly from &mut [Handle<JsValue>] to &mut [napi_value]. This does not require consuming the Vec or making any copies.

[dherman]

I really like this API. This is a big improvement. I think it can be optimized further if we don't tie ourselves to using the existing function call implementation and instead go directly to Node-API.

...

It should be safe to transmute directly from &mut [Handle<JsValue>] to &mut [napi_value]. This does not require consuming the Vec or making any copies.

I made these changes but didn't literally transmute the &[], I used .as_ptr() instead.

CI is mostly passing, I just need to fix linting errors…

[dherman]

I just realized Call::new() isn't generic in the result type the way JsFunction::construct() is. I'll address that soon.

[dherman]

Ready for re-review when you have a chance, @kjvalencik 🙏

[kjvalencik]

I made these changes but didn't literally transmute the &[], I used .as_ptr() instead.

Ah! That's right, this is a raw pointer, so as performs the same function. We're still effectively doing the same thing. It might be useful sometime in the future to add a static assertion to ensure they actually have the same layout.

[kjvalencik]

This could use a few tests on new functionality. For example, testing non-unit args tuples.

[dherman]

@kjvalencik Looking at usage code in the tests yesterday, I didn't quite like where we'd landed with the API. Having builder methods like JsFunction::args() is really confusing when we also have Call::args() and FunctionCall::args() and even the unrelated but similarly-named FunctionContext::arguments(). I saw code like:

    let o = cx
        .argument::<JsFunction>(0)?
        .arg(cx.number(0.0))
        .construct(&mut cx)?;

and it's especially confusing to see the .argument() and .arg() back to back. I've pushed a revision to the API with a few new concepts:

• I refer to the generic concept of either calling or constructing a function as "invocation".
• The only way to get a builder is JsFunction::apply(), and there are no builder shortcut methods on JsFunction.
  • There's a rough analogy there to Function.prototype.apply() where, since this is Rust, we're constructing the arguments with a builder rather than an array.
  • Alternatively we could name this .invoke() or .invocation() but that feels more obfuscated to me.
• I renamed the builders to InvocationBuilder for the general one and CallBuilder for the one that doesn't have .construct().

This makes examples like the above one clearer:

    let o = cx
        .argument::<JsFunction>(0)?
        .apply()
        .arg(cx.number(0.0))
        .construct(&mut cx)?;

Even thought it's slightly less concise, when you're already method chaining I don't feel like it makes a huge difference. And I found one-liners often still fit on one line since .apply() is only 8 more characters.

I also think this makes the docs clearer, which gives me some confidence that it's an improvement:

Screenshot

But very open to feedback!

[kjvalencik]

@dherman That's some really great points! I'm convinced.

I think apply is a better choice than anything we've previously discussed. One last option I wanted to throw out and get your opinion on is apply_with(). It's not quite as terse, but it might better imply that we aren't done yet.

[dherman]

Following up on a Slack conversation @kjvalencik and I had: his apply_with() and ApplyOptions idea led me to realize that the whole typestate design was kind of silly, and instead we could just have the programmer choose up front if they are doing a call or a new. Then we can call them call_with() and CallOptions, and construct_with() and ConstructOptions. And we can use .apply() as the final "now go do it" method, in keeping with the analogy to Function.prototype.apply and to avoid redundancy. This ends up reading quite nicely:

let o = cx
    .call_with()
    .arg(cx.number(0.0))
    .apply(&mut cx);

[dherman]

@kjvalencik Should be ready for another review! I moved the new types into neon::types::function but didn't make the changes we talked about with aliasing neon::object to neon::types::object and hiding the old location from the docs. I'll do that in a separate PR.

[dherman]

I'm going to close this for now since I submitted #817 to implement part of this functionality (in particular the Arguments trait) first. When that merges I'll follow up with a builder API PR.