Proposal: gloo-properties - mid-level API for getting and setting properties

rustwasm / gloo

A modular toolkit for building fast, reliable Web applications and libraries with Rust and WASM

https://gloo-rs.web.app

Apache License 2.0

1.78k stars 145 forks source link

Proposal: gloo-properties - mid-level API for getting and setting properties #53

Open samcday opened 5 years ago

samcday commented 5 years ago

Summary

A mid-level API to set and query properties on arbitrary Javascript values.

Motivation

The js-sys crate already provides Reflect to get and set properties on JsValue types, and wasm-bindgen provides Serde integration that allows to easily map an existing Rust type into Javascript land. So why do we need more?

The use case we're addressing here is ad-hoc property access. This occurs often when writing Rust WASM code that interfaces with existing userland Javascript APIs. It's expected that as the Rust WASM ecosystem matures, this situation will decline in frequency. However that kind of shift is not going to happen overnight, and in the meantime we should provide better ergonomics for users that need to interface with raw Javascript values.

Detailed Explanation

`gloo_properties::property`

Used to get/set a single property on an existing JsValue.

API:

/// Retrieves a single Property on target Object. This Property can be used to
/// get and set the value.
pub fn property<K>(target: &JsValue, key: K) -> Property
    where K: Into<JsValue> { /* ... */ }

struct Property { /* ... */ }
impl Property {
    /// Retrieves the value of this Property as a String. If the property is
    /// undefined, null, or not a String, an Err is returned.
    pub fn string(&self) -> Result<String, JsValue> { /* ... */ }

    /// Retrieves the value of this Property as a u32. If the property is
    /// undefined, null, not a Number, or overflows a u32, an Err is returned.
    pub fn u32(&self) -> Result<u32, JsValue> { /* ... */ }
    // And so on...
    pub fn u16(&self) -> Result<u16, JsValue> { /* ... */ }
    // pub fn u8...
    // pub fn i32...
    // pub fn i16...
    // pub fn i8...

    /// Retrieves the value of this Property as a bool. If the property is
    /// undefined, null, or not a Boolean, an Err is returned.
    pub fn bool(&self) -> Result<bool, JsValue> { /* ... */ }

    /// Retrieves the value of this Property as any kind of Javascript mapped
    /// type. Anything that is exported from #[wasm_bindgen] is usable. If the
    /// property is undefined, null, or an incorrect type, an Err is returned.
    pub fn cast<T: JsCast>(&self) -> Result<T, JsValue> { /* ... */ }

    /// Retrieves a sub-property from this Property. This is used to traverse
    /// arbitrary nested object structures. Note that if a property does not
    /// exist, a Property will still be returned, but any attempts to get or set
    /// a value will return an Error.
    pub fn property<K: Into<JsValue>>(self, key: K) -> Property { /* ... */ }

    /// Sets the given value on the Property. Returns an error if the set failed
    /// for any reason. e.g frozen object, error in a setter on the Object, etc.
    pub fn set<V: Into<JsValue>>(&self, val: V) -> Result<bool, JsValue> { /* ... */ }
}

Usage examples:

use gloo_properties::property;

// Because key is generic over Into<JsValue>, using it is very convenient:
property(&obj, "foo");           // static string literals
property(&obj, &existing_str);   // existing String / &strs
property(&obj, existing_str);    // Pass by ref not required.

// Symbols work too.
let sym = JsValue::symbol("secret");
property(&obj, &sym);

// Nested property access:
property(&obj, "foo").property("bar").property("quux");

// Retrieving values.
property(&obj, "foo").string().unwrap_throw();
let my_element: web_sys::Element = property(&obj, "foo").cast();

// Traversing multiple levels never fails until trying to get/set something.
property(&obj, "i").property("don't").property("exist"); // -> Property
property(&obj, "exists").property("nope").string() // -> Err("Property 'i' does not exist")

// Setter is generic over anything that can convert into a JsValue, which is
// all core Rust types, and all web_sys types.
property(&obj, "foo").set("a direct string");
property(&obj, "foo").set(42u32);
property(&obj, "foo").set(true);

let an_existing_object = Object::new();
property(&obj, "foo").set(an_existing_object);

`gloo_properties::setter`

Used to quickly build up a set of properties on a value.

API:

/// Returns a Setter that can be used to set multiple properties on the given
/// target. Ownership of the target is taken until Setter#finish() is called.
pub fn setter<T: JsCast>(target: T) -> Setter<T> { /* ... */ }

pub struct Setter<T: JsCast> { /* ... */ }
impl<T: JsCast> Setter<T> {
    /// Sets given key/value pair on the target object.
    pub fn set<K, V>(self, key: K, val: V) -> Self
    where
        K: Into<JsValue>,
        V: Into<JsValue>,
    { /* ... */ }

    /// Sets given key/value pair on the target object, and then calls the given
    /// closure to set sub properties on the provided value.
    pub fn with<K, V, F>(self, key: K, val: V, func: F) -> Self
    where
        K: Into<JsValue>,
        V: JsCast,
        F: FnOnce(Setter<V>) -> Setter<V>,
    { /* ... */ }

    /// Finishes setting properties on target object and returns it.
    pub fn finish() -> T { /* ... */ }
}

Usage:

use gloo_properties::setter;

let my_sym = JsValue::symbol("secret");

let my_obj = setter(Object::new())
    .set("foo", "bar")
    .set(&my_sym, "quux")
    .set(123, 321)
    .set("bool", false)
    .set("jsval", Object::new())
    .with("nested", Object::new(), |sub| sub
        .set("and", "so")
        .set("on", "it goes"))
    .finish();

Drawbacks, Rationale, and Alternatives

Drawbacks

Currently, working with Reflect (which this crate would use) is slower than working with duck-typed interfaces, and in many cases is also slower than the Serde integration.

Alternatives

The obvious alternatives are to use duck-typed interfaces or the Serde integration. Both of these approaches require one to write and annotate a struct with the desired fields. The proposed API is suitable for use when the user does not wish to define explicit types in order to grab properties out of an existing value in an ad-hoc manner.

For example:

// Instead of needing to do this:
{
    #[wasm_bindgen]
    extern "C" {
        type MyThing;

        #[wasm_bindgen(method, getter, structural)]
        fn foo(this: &MyThing) -> String;
    }
    let thing: MyThing = my_obj.dyn_into::<MyThing>().unwrap_throw();
    thing.foo
}

// Or this:
{
    #[derive(Deserialize)]
    struct MyThing {
        foo: String,
    }
    let thing: MyThing = JsValue::into_serde(&my_obj).unwrap_throw();
    thing.foo
}

// One can just do this:
{
    property(&my_obj, "foo").string().unwrap_throw();
}

Unresolved Questions

Currently, none.

Pauan commented 5 years ago

Relatedly, we should have convenient APIs for looking up keys on existing objects (since that's actually a really common thing to do, and it's quite nasty right now!)

fitzgen commented 5 years ago

Thanks for writing this up!

Questions off the top of my head:

How would this interface support symbols and i32 property keys?
Whenever I've needed to work with raw JS objects, I've generally found it easier to use duck-typed interfaces (for example: the definition here and the usage here) -- what utility would this interface provide over using duck-typed interfaces?
ObjectProps getters -- are these unwraping the type conversions and panicking on failure?

Aloso commented 5 years ago

Here's an idea for a possibly better API:

Create a trait IntoJs for converting an object to a JsValue, which is implemented for strings, numbers, bool, char, JsValue, Object and maybe more.

Then lets create a function

pub fn set_property<K: IntoJs, V: IntoJs>(object: &Object, key: K, value: V) {
    Reflect::set(object, key.into_js(), value.into_js()).unwrap_throw();
}

and a macro using this function that can be used like this:

let my_obj = set_props! { Object::new(), with
    "foo"   -> "bar",
    "quux"  -> 123u8,
    "b0rk"  -> 123.1,
    "meep"  -> any_old_JsValue,
    "blerg" -> set_props!(Object::new(), with "potato" -> "spaghetti"),
};

This can also be used to update to an existing object:

set_props! { my_obj, with
    "foo"   -> 123u8,
    "quux"  -> "bar,
};

Aloso commented 5 years ago

I tried it out, and it works with a recursive macro_rules! macro.

Show code

```rust pub trait IntoJs { fn into_js(&self) -> JsValue; } pub fn set_property(object: &Object, key: K, value: V) { Reflect::set(object, &key.into_js(), &value.into_js()).unwrap_throw(); } #[macro_export] macro_rules! set_props { ( $obj:expr, with $($props:tt)* ) => { { let obj = $obj; set_props_internal!(obj; $($props)*); obj } }; } macro_rules! set_props_internal { ( $obj:ident; $key:literal -> $val:expr, $($rest:tt)* ) => { set_property(&$obj, $key, $val); set_props_internal!($obj; $($rest)*); }; ( $obj:ident; $key:literal -> $val:expr ) => { set_property(&$obj, $key, $val); }; () => {}; } impl IntoJs for &str { fn into_js(&self) -> JsValue { JsValue::from_str(self) } } impl IntoJs for Object { fn into_js(&self) -> JsValue { JsValue::from(self) } } impl IntoJs for JsValue { fn into_js(&self) -> JsValue { self.clone() } } macro_rules! impl_into_js_num { ( $($t:ty)* ) => { $( impl IntoJs for $t { fn into_js(&self) -> JsValue { JsValue::from_f64(*self as f64) } } )* } } impl_into_js_num!(u8 i8 u16 i16 u32 i32 u64 i64 u128 i128 f32 f64); ```

samcday commented 5 years ago

@Pauan yeah, absolutely. The initial ideas I have around ObjectProps are somewhat nascent, but as you've rightly pointed out, querying data out of ad-hoc objects from Rust-side is a little verbose right now.

@fitzgen very good points, let me address each one.

I'm not really a Rust guru so I don't know if there's some super idiomatic Rust-y way to accomplish handling Symbols / i32 keys in the main API I already sketched out. I believe (though I don't have any hard numbers or telemetry to back this up) that the vast majority of use cases in dealing with JS-land objects are gonna be dealing with standard String based property names. We could also just have a complete copy of the API I outlined with _sym and _i32 suffixes. Open to ideas here!

The purpose of ObjectBuilder is really to handle the ad-hoc object building case, for example when talking to existing JS libraries/userland code. i.e situations where you're only going to build that kind of object in one place. If we take my original ObjectBuilder example again, here's how it would look with duck typed interfaces:

  extern "C" {
    #[derive(Debug, Clone)]
    pub type Bacon;

    #[wasm_bindgen(structural, method, setter)]
    fn set_foo(this: &Bacon, val: &str);

    #[wasm_bindgen(structural, method, setter)]
    fn set_quux(this: &Bacon, val: u64);

    #[wasm_bindgen(structural, method, setter)]
    fn set_bleep(this: &Bacon, val: i64);

    #[wasm_bindgen(structural, method, setter)]
    fn set_b0rk(this: &Bacon, val: f64);

    #[wasm_bindgen(structural, method, setter)]
    fn set_meep(this: &Bacon, val: JsValue);

    #[wasm_bindgen(structural, method, setter)]
    fn set_blerg(this: &Bacon, val: &SubBacon);

    #[derive(Debug, Clone)]
    pub type SubBacon;

    #[wasm_bindgen(structural, method, setter)]
    fn set_potato(this: &Bacon, val: &str);
  }

  let my_obj: Bacon = Object::new().unchecked_into::<Bacon>();
  my_obj.set_foo("bar");
  my_obj.set_quux(123u8);
  my_obj.set_bleep(321i16);
  my_obj.set_b0rk(123.1);

  let my_sub_obj: SubBacon = Object::new().unchecked_into::<SubBacon>();
  my_sub_obj.set_potato("spaghetti");

  my_obj.set_blerg(my_sub_obj);

As you can see, there's still quite a lot of verbosity here. If we're only ever building this object once to send it out to some Javascript API somewhere, it feels redundant to have built all this typing information manually. I think the duck typing interface is the way to go when you're dealing with incoming parameters FROM the JS side. In that case you might have clients passing in an object that has properties + callback methods and you want a nice interface over all of that.

Yes, the idea behind ObjectProps would be to immediately unwrap_throw. I think that this addresses a majority of use-cases in when you'd even be accessing properties from a Javacript object in the first place. Once you're in WASM land, you're dealing with Rust types and all the richness that the Rust type system has to offer. It's only when you're receiving ad-hoc objects from the Javascript side that you need to roll up your sleeves and do the messy type coercions. In those situations, I'm making the case that 9 times out of 10 you're not going to want to do anything more than simply unwrap_throw anyway, since if the JS side passed in a bad type for a property, you don't usually want to proceed any further. In the situations where you do have the ability to work around some odd shape of data passed in, then you can still easily drop down to the lower level API available in js_sys/wasm_bindgen. That said, I haven't thought through ObjectProps as much, I'm sure there's a lot more useful stuff that it could be doing. For example we could have variants of the API that do some basic coercion that follows Javascript rules. i.e asking for a string prop will .toString() whatever properties does exist, so you'll get a string representation of Number/bools, etc.

@Aloso that's very cool! That approach with a macro would probably also provide a cleaner way to address @fitzgen's question regarding support for Symbol and i32 keys right? Rust macros still kinda scare me when trying to conceptualize what they can do and what they can't. For example would macros allow us to do something like this?

let my_obj = set_props! { Object::new(), with
    "foo"   -> "bar",
    Symbol("spaghetti") -> "potato",
    42 -> "meaning",
};

I'm also curious to see what others think about macros vs. a more vanilla API comprised of functions. For me personally, I don't reach for macros unless they allow me to express something that is otherwise very difficult or verbose to express in normal Rust syntax.

I don't think we need all those variants of impl_into_js_num, Into<u64> + Into<i64> variants should be all that's necessary (since all lower int primitives implement Into<u64>):

fn int<T: Into<i64>>(val: T) -> JsValue {
    JsValue::from_f64(val.into() as f64)
}
fn uint<T: Into<u64>>(val: T) -> JsValue {
    JsValue::from_f64(val.into() as f64)
}
fn float<T: Into<f64>>(val: T) -> JsValue {
    JsValue::from_f64(val.into())
}

// These all work:
uint(123u8);
uint(321u16);
int(666i32);
float(3.14159f32);

samcday commented 5 years ago

@Aloso another great point you had is being able to augment properties onto existing Objects. If we opted to not go for the macro approach and stick with the existing ObjectBuilder then that's pretty easy to handle:

  let existing_obj = ObjectBuilder::new().build();
  let same_obj = ObjectBuilder::from(existing_obj).string("foo", "bar").build();

Aloso commented 5 years ago

@samcday

For example would macros allow us to do something like this?

Yes. macro_rules! doesn't allow the -> token after a expr, so I used a literal on the left. This doesn't work for symbols, but we could also allow ident, Symbol(literal) and Symbol(ident), which would cover most use cases.

Pauan commented 5 years ago

Yes, the idea behind ObjectProps would be to immediately unwrap_throw.

I think there's a pretty simple solution: the build() method would return Result, and then a new unwrap_build() method is added which calls unwrap_throw():

#[inline]
fn unwrap_build(self) -> Object {
    self.build().unwrap_throw()
}

Rust macros still kinda scare me when trying to conceptualize what they can do and what they can't.

Rust macros can do pretty much anything, since they just operate on a token stream.

There's a few limitations, which you can read about here.

For the most part I don't worry about it: I just make whatever syntax seems reasonable, and then afterwards fix up any compile errors that Rust gives me.

I don't think we need all those variants of impl_into_js_num, Into<u64> + Into<i64> variants should be all that's necessary

i64, u64, i128, and u128 cannot (currently) be supported (they cannot fit into an f64).

For performance reasons, it's probably best to handle each type separately (rather than using Into)

samcday commented 5 years ago

I think there's a pretty simple solution: the build() method would return Result, and then a new unwrap_build() method is added which calls unwrap_throw():

Are you getting ObjectProps mixed up with ObjectBuilder? I'm not sure if there's any need to force users to handle exception cases while building an Object with ObjectBuilder, because the API is strongly typed and is essentially mapping Rust stuff into JS-land, which can handle anything.

The only edge-case I can think of is if one tried to add some properties onto an existing Object that was frozen/sealed, or had custom setters for property names you were trying to set. Those cases might be enough justification to not support feeding a pre-existing Objects into ObjectBuilder if we want to keep its API lean and focused.

i64, u64, i128, and u128 cannot (currently) be supported (they cannot fit into an f64).

Ah yes good point. I mean in that case we can lower it down to Into<u32|i32> instead right?

For performance reasons, it's probably best to handle each type separately (rather than using Into)

Hmm, do you have more background on this concern? I'm not sure that widening an int is a particularly expensive operation, and it has to happen anyway since the only number primitive that Javascript supports is f64 anyway right?

RReverser commented 5 years ago

When you want to avoid the hop of encoding a Rust type to JSON and then deserializing it again. That said, this might actually be faster for complex object graphs, depending on how much overhead there is in the ping-ponging between WASM and JS land using ObjectBuilder.

Note that this will be fixed and is tracked separately in https://github.com/rustwasm/wasm-bindgen/issues/1258.

I'm currently working on an [for now internal] solution for it, got it to the feature-complete stage, and now investigating performance.

So far it looks like, even with pre-encoding object keys and so on, on some inputs it can be much slower than serde-json due to the overhead of frequent crossing of boundary and encoding lots of small strings as opposed to a single large one, but on other inputs involving more numbers than strings it can be much faster. But yes, hopefully:

ObjectBuilder would always be faster once host bindings land

Either way, I would bet on that existing serde integration rather than try and introduce yet another API for objects.

samcday commented 5 years ago

Thanks for the feedback @RReverser - what you're working on sounds very interesting and I'll be sure to check it out when you put some code up

Either way, I would bet on that existing serde integration rather than try and introduce yet another API for objects.

You might be right, although your feedback only touched on one of the points I raised. What are your thoughts on these other situations?

When you care about object identity (i.e ===)
When you're dealing with ArrayBuffers

RReverser commented 5 years ago

When you're dealing with ArrayBuffers

This is also covered by proper serde integration - it has dedicated methods for serializing / deserializing bytes (usually via https://github.com/serde-rs/bytes) and I'm already leveraging that for creating and passing back Uint8Array.

When you care about object identity

This one seems pretty rare usecase and indeed harder to solve... I guess it could be possible to implement that somehow with {de}serialize_with and custom checks down the road, but feels a bit niche for now.

samcday commented 5 years ago

Of course performance is an important aspect here, but when I think about this more I think the main purpose of ObjectBuilder would be to provide better ergonomics for the ad-hoc object building case. As discussed previously, if one is building an object for JS-land in multiple places or is expecting to receive a very particular shape of object from JS-land, then the existing tools (structural tags / JsCast / JsValue::into_serde) make more sense.

The question is whether the situation I describe is considered common enough to warrant the API surface area in Gloo. I would make the case that it is - which is why I raised this RFC :). i.e if you're surgically inserting Rust WASM into parts of an existing codebase, you'll be dealing with all sorts of ad-hoc objects everywhere. needing to define structs in Rust land for each kind of ad-hoc object (so that it can either be tagged with structural tags or fed into the Serde integration) is the sticking point for me here.

Concrete example that I'm dealing with right now in my own project: passing messages from a Web Worker written in Rust. There's a bunch of messages that get exchanged between the main UI and the Rust Web Worker. Each message is different but only shows up once in the message handling code, at which point I scoop all the relevant data out and call into proper Rust methods.

Using Serde or duck typed interfaces looks like this:

struct MessageA {
  blah: String
}
struct MessageB {
  foo: u32
}
//..
struct MessageZ {
  bar: bool,
}

fn send_message_a() {
  let msg = MessageA{blah: "blorp"};
  worker().postmessage(&JsValue::from_serde(msg));
}

fn send_message_b() {} // and so on

Whereas with the proposed ObjectBuilder you're just doing this:

fn send_message_a() {
  let msg = ObjectBuilder::new().string("blah", "blorp").build();
  worker().postmessage(&msg);
}
fn send_message_b() {} // and so on

samcday commented 5 years ago

I have a habit of being kinda verbose when I'm trying to get a point across. Let me try and distill it into a much easier TL;DR:

I think that requiring users to crystallize every ad-hoc Object they need to build for interop between Rust and JS into a struct is not user-friendly. I think the situations where one needs to do such a thing is frequent enough to warrant adding something like ObjectBuilder to the mix alongside the existing options.

I'd like to know what others think!

RReverser commented 5 years ago

I see. It sounds like your main concern is the boilerplate associated with defining custom types for one-off objects, even if objects themselves are still static. If so, you should be able to hide implementation details in a macro like:

macro_rules! object {
  ($($prop:ident: $value:expr),* $(,)*) => {{
    #[derive(Serialize)]
    pub struct Object<$($prop,)*> {
      $($prop: $prop,)*
    }

    JsValue::from_serde(&Object {
        $($prop: $value,)*
    })
  }};
}

and then reuse it wherever you need ad-hoc objects like

fn send_message_a() {
  let msg = object! { blah: "blorp" };
  worker().postmessage(&msg);
}

This way you should get the best of both worlds (static serialization + no need to write out types) with relatively low efforts. What do you think?

Aloso commented 5 years ago

i64, u64, i128, and u128 cannot (currently) be supported (they cannot fit into an f64).

All Rust integers fit into a f64. The biggest finite f64 is ≃ 1.7976931348623157 × 10³⁰⁸, whereas the biggest u128 is only ≃ 3.402823669209385 × 10³⁸. Although casting a i64 to f64 involves rounding, I see no reason to forbid that.

@RReverser

When you're dealing with ArrayBuffers

This is also covered by proper serde integration - it has dedicated methods for serializing / deserializing bytes

I thought that objects such as ArrayBuffers can be passed between JS and Rust, without (de)serializing at all? I understand that strings need to be converted (UTF-8 <=> UTF-16), but not ArrayBuffers. So I'm guessing that (de)serializing an object containing a large ArrayBuffer is always slower than using Reflect. Or am I understanding something wrong?

Also, how does the (de)serialization work if the JsValue contains cycles?

you should be able to hide implementation details in a macro like:
macro_rules! object { ... }

This looks really nice. The only missing feature is numeric and symbol properties, like

object! {
    "foo" : bar,
    4 : true,
    Symbol("hello") : "world",
}

And I still think that a macro (or similar API) would be handy to add or modify values of an existing JsValue, even if that requires you to call unwrap_throw(), e.g.

extend! { div, with
    "id": "my-div",
    "className": "fancy border red",
}.unwrap_throw();

RReverser commented 5 years ago

I thought that objects such as ArrayBuffers can be passed between JS and Rust, without (de)serializing at all?

Unfortunately not, you still need to copy the memory from JS to WASM or the other way around.

Also, how does the (de)serialization work if the JsValue contains cycles?

By default it doesn't, but then, it's not particularly easy to create cycles in Rust structures anyway (even with Rc you will end up leaking memory).

This looks really nice. The only missing feature is numeric and symbol properties, like

I don't think these are common cases (you pretty much never should create an object with numeric properties), but they shouldn't be hard to add where necessary.

And I still think that a macro (or similar API) would be handy to add or modify values of an existing JsValue

Any reason you can't use Object::assign(...) for that?

Object::assign(div, object! {
  id: "my-div",
  className: "fancy border red"
})

samcday commented 5 years ago

Unfortunately not, you still need to copy the memory from JS to WASM or the other way around.

Not strictly true, see TypedArray::view. You can take any arbitrary block of memory in Rust side and cast it into a typed array. Under the hood this is taking the underlying WebAssembly.Memory that the program is running in, slicing it to the bounds of the given Rust slice, and then returning the backing ArrayBuffer view available in Memory.buffer. This is zero copy. Of course doing such a thing can be dangerous (which is why view is unsafe). But, it can be done, and I'm sure people are doing it in hot paths.

If so, you should be able to hide implementation details in a macro like:

Yeah, @Aloso suggested a macro also, and it looks similar to your suggestion. The difference is you're proposing to build a struct as part of that macro.

My question is, are you suggesting that such a macro is something people should just be using themselves when needed, or are you suggesting that it's worth codifying here in Gloo? If the former, then yeah I think we can just find a nice place in the documentation to wedge that snippet and call it a day.

If the latter, then we can keep iterating on your suggestion. Instead of using Serde under the hood, we can also annotate the generated struct fields with the structural tags and use direct setters, which will handle the object identity stuff I raised and also support zero copy references of ArrayBuffers.

RReverser commented 5 years ago

Of course doing such a thing can be dangerous (which is why view is unsafe). But, it can be done, and I'm sure people are doing it in hot paths.

It can't be done (safely) as part of any object serialisation, because literally the next property can cause an allocation that will invalidate the view. It's meant to be used only as a temporary short-lived reference, which makes it suitable for copying data to JS.

My question is, are you suggesting that such a macro is something people should just be using themselves when needed, or are you suggesting that it's worth codifying here in Gloo?

I don't have particularly strong opinion. In my personal experience, arbitrary objects like these are pretty infrequent compared to repetitive typed objects, so it doesn't feel like something that belongs in the core yet, but maybe could be shared as a separate crate (especially since, as you noted, there are various potential implementations which could go into different crates).

But, if most people disagree, I defer.

we can also annotate the generated struct fields with the structural tags

AFAIK these are not necessary and are the default these days, but otherwise yeah, that's a good idea too.

also support zero copy references of ArrayBuffers.

As mentioned above, this is still highly unsafe and likely won't work here.

samcday commented 5 years ago

Okay so I wrote an entirely unscientific set of benchmarks to test the overhead of building a simple {"foo": "bar"} object. Here's the code:

lib.rs

use js_sys::*;
use serde::Serialize;
use wasm_bindgen::prelude::*;
use wasm_bindgen::JsCast;

struct ObjectBuilder {
    obj: Object,
}

impl ObjectBuilder {
    fn new() -> ObjectBuilder {
        let obj = Object::new();
        ObjectBuilder { obj }
    }

    fn string(self, prop: &str, val: &str) -> Self {
        Reflect::set(&self.obj, &JsValue::from_str(prop), &JsValue::from_str(val)).unwrap_throw();
        self
    }

    fn build(self) -> JsValue {
        self.obj.unchecked_into()
    }
}

// Called by our JS entry point to run the example.
#[wasm_bindgen]
pub fn objectbuilder(array: &Array) {
    for _ in 0..1000 {
        array.push(&ObjectBuilder::new().string("foo", "bar").build());
    }
}

#[wasm_bindgen]
pub fn noop(array: &Array) {
    if array.length() != 0 {
        panic!("preventing optimizations, I think? {:?}", array);
    }
}

#[wasm_bindgen]
pub fn empty_obj(array: &Array) {
    for _ in 0..1000 {
        array.push(&Object::new());
    }
}

#[wasm_bindgen]
pub fn serde(array: &Array) {
    #[derive(Serialize)]
    struct SerdeObject<'a> {
        foo: &'a str,
    }
    for _ in 0..1000 {
        let obj = SerdeObject { foo: "bar" };
        array.push(&JsValue::from_serde(&obj).unwrap());
    }
}

#[wasm_bindgen]
pub fn ducky(array: &Array) {
    #[wasm_bindgen]
    extern "C" {
        type DuckObject;

        #[wasm_bindgen(method, setter, structural)]
        fn set_foo(this: &DuckObject, val: &str);
    }
    for _ in 0..1000 {
        let obj = Object::new().unchecked_into::<DuckObject>();
        obj.set_foo("bar");
        array.push(&obj);
    }
}

index.js

function verifyResult(array) {
  if (array.length !== 1000) {
    throw new Error(`Invalid length: ${array.length}`);
  }
  for (const item of array) {
    if (item.foo !== 'bar') {
      throw new Error(`Bad item: ${JSON.stringify(item)}`);
    }
  }
}

function runTest(label, fn, skipVerify) {
  const results = [];
  let array = new Array(1000);

  for (let i = 0; i < 100; i++) {
    array.length = 0;
    let start = performance.now();
    fn(array);
    let end = performance.now();
    if (!skipVerify) {
      verifyResult(array);
    }
    results.push(end-start);
  }

  return {
    test: label,
    avg: results.reduce((acc, val) => acc + val, 0) / results.length,
    worst: Math.max.apply(Math, results),
    best: Math.min.apply(Math, results),
  };
}

import("../crate/pkg").then(module => {
  const results = [
    runTest('js', array => {
      for (let i = 0; i < 1000; i++) {
        const obj = new Object();
        obj.foo = "bar";
        array.push(obj);
      }
    }),

    runTest('rustland: noop', module.noop, true),
    runTest('rustland: empty_obj', module.empty_obj, true),
    runTest('rustland: objectbuilder', module.objectbuilder),
    runTest('rustland: serde', module.serde),
    runTest('rustland: ducky', module.ducky),
  ];

  console.table(results);
});

If you spot any glaring errors then please point them out. Even though this is a very coarse test, it's pretty revealing, I think. The results I get in Chrome look like this:

Test	Avg	Worst	Best
js	0.3636000060942024	3.810000023804605	0.13499998021870852
rustland: noop	0.0024500000290572643	0.11999998241662979	0
rustland: empty_obj	0.43634999776259065	1.7300000181421638	0.3249999135732651
rustland: objectbuilder	3.8228000001981854	12.665000045672059	2.905000001192093
rustland: serde	8.599249996477738	20.55000001564622	8.019999950192869
rustland: ducky	1.7073499981779605	12.589999940246344	1.4550000196322799

There's a lot of variance between runs because I have no idea how to write a good micro benchmark for the browser, but the variance isn't so much that is takes away from the general scale of difference between the classes of tests.

Anyway, using a duck typed interface appears to be the clear choice when building small objects. ObjectBuilder dispatching through Reflect::set even once is enough to make it substantially (nearly 2x) slower than using duck typed interfaces.

My conclusions from this are:

Obviously JS was going to be faster because the JIT can clearly see the shape of the object we're building in the inner loop and would be doing all sorts of fun stuff to optimize that.
Serde is surprisingly slow for this scenario, but hopefully it gets faster with @RReverser's work.
Right now, building Objects from Rust side is just painfully slow and should probably be avoided at all costs in performance-sensitive hot paths. This is exactly the sort of thing Host Bindings will make more-betterer, right?
I'm closing out this RFC since I don't think there's any value in the ObjectBuilder proposal as compared to existing solutions

samcday commented 5 years ago

Okay slight addendum. I knew something felt a little weird about the results I saw seeing. I was compiling the crate in development mode. Heh.

Results in releasemode:

Test	Avg	Worst	Best
js	0.16065000323578715	0.47500000800937414	0.12999994214624166
rustland: noop	0.0019499973859637976	0.09999994654208422	0
rustland: empty_obj	0.1994499983265996	5.755000049248338	0.07499998901039362
rustland: objectbuilder	3.075149995274842	22.119999979622662	2.3449999280273914
rustland: serde	2.3486500151921064	12.519999989308417	1.879999996162951
rustland: ducky	1.3619999960064888	12.400000006891787	1.0900000343099236

So Serde is nowhere near as slow as I was stating in my previous comment. It's still (currently?) slower than duck-type interfaces though. And I still stand by my conclusion that right now, there's a large amount of overhead in building objects in Rust side, such that it should be avoided where possible.

RReverser commented 5 years ago

There's a lot of variance between runs because I have no idea how to write a good micro benchmark for the browser

The common go-to library is Benchmark.js (that's what I used for years at least, and that's what's been used on JSPerf).

Anyway, using a duck typed interface appears to be the clear choice when building small objects. ObjectBuilder dispatching through Reflect::set even once is enough to make it substantially (nearly 2x) slower than using duck typed interfaces.

In case you're interested, one of the many small cuts I found and fixed as part of serde-wasm-bindgen work is that in V8 Reflect.set is much slower than direct property setter, so I ended up defining custom import type for Object with an indexing_setter method - even that on its own made 1.5x-2x difference in some benchmarks.

This is exactly the sort of thing Host Bindings will make more-betterer, right?

Depends on your type of data. If you have to deal with lots of strings in your objects, then in such benchmarks string encoding/decoding becomes the most expensive part of the profile and there is not much you can do there, unfortunately, either with host bindings or without them.

But I agree with your conclusions - if you can define your object statically with setters, it's always going to be faster than serialisation.

Would you be willing to adapt the macro to use your approach and publish it on crates.io for others?

samcday commented 5 years ago

The common go-to library is Benchmark.js

Ah yes of course. In the back of my mind I was thinking "I wish I could just use whatever JSPerf is using" but didn't bother to see if that was easily possible ;) Benchmarking is a fickle science for sure, but I wasn't looking for extreme accuracy here, just a reasonably apples-to-apples comparison of the different approaches. I think what I quickly whipped up does that reasonably well enough.

In case you're interested, one of the many small cuts I found and fixed as part of serde-wasm-bindgen work is that in V8 Reflect.set is much slower than direct property setter, so I ended up defining custom import type for Object with an indexing_setter method - even that on its own made 1.5x-2x difference in some benchmarks.

That's very interesting! And would probably go a long way in explaining the difference between the rustland: ducky and rustland: objectbuilder tests. I wonder if Reflect::set is so much slower because it circumvents the JITs ability to lower the foo property down to a primitive or something? I dunno, a VM engineer I am not.

Would you be willing to adapt the macro to use your approach and publish it on crates.io for others?

No, I don't think so. If it's worth putting in a crate then it's worth putting in Gloo (IMO). But I just don't think it's worth putting in a crate. Building a struct for objects, even if used once, isn't that bad.

I do wonder if there's some work that could be done in the documentation here though. Before I made this proposal I didn't know that the duck typing approach could be applied so easily to setting arbitrary object properties. I guess I didn't read the documentation well enough but maybe there's room for some kind of "cookbook" section that walk through common and simple use cases like this? :man_shrugging:

Pauan commented 5 years ago

@Aloso All Rust integers fit into a f64.

No, they don't. The largest integer representable by an f64 is 9007199254740991, which is 2.0.powf(53.0) - 1.0 (53 bits)

Above that number, floating points lose precision. For example, 2.0.powf(53.0) is equal to 2.0.powf(53.0) + 1.0

Floating points are complicated, but here is a good summary. The Wikipedia article is also rather good.

It is simply not possible to fit an u64 (or higher) into an f64. In the case of u128 there isn't even enough bits: f64 has 64 bits, whereas u128 requires 128 bits.

And no, silently truncating or rounding or causing other such lossiness isn't okay. That should require explicit instructions from the user, and shouldn't happen automatically.

@RReverser Depends on your type of data. If you have to deal with lots of strings in your objects, then in such benchmarks string encoding/decoding becomes the most expensive part of the profile and there is not much you can do there, unfortunately, either with host bindings or without them.

Host bindings have support for directly passing UTF-8 strings, which (hopefully) will avoid encoding/decoding.

RReverser commented 5 years ago

Host bindings have support for directly passing UTF-8 strings, which (hopefully) will avoid encoding/decoding.

Ah interesting. Need to see how it's going to become implemented, but sounds promising if they could create strings backed by WASM memory.

rylev commented 5 years ago

No, I don't think so. If it's worth putting in a crate then it's worth putting in Gloo (IMO). But I just don't think it's worth putting in a crate. Building a struct for objects, even if used once, isn't that bad.

@samcday I've read the entire thread and I don't fully understand how you came to this conclusion. I had an instance today where I needed to create a JSValue (in the form of a JS object) in a code path that wasn't hot. Having a convenience macro would have helped me out. It wasn't terribly inconvenient, but I don't believe the purpose of gloo is to provide "not terrible APIs" - I think web_sys/js_sys mostly already accomplish this.

samcday commented 5 years ago

@rylev thanks for the feedback!

I guess I'm not really that familiar with how the RFC process works just yet. My feeling was that if initial feedback was lukewarm then it's probably not something that should be pursued, especially considering there's approximately a trillion other useful things that could be added to Gloo right now. Have you got some tips on how to judge if/when an RFC should be pushed?

I'm happy to keep going with this one if there's enough appetite for it.

rylev commented 5 years ago

It's probably best to have @fitzgen weigh in here. I'm just of the opinion that creating ad-hoc JavaScript objects is something users may want to do. I can see that argument that as the use of web_sys goes down (because people use the gloo wrappers more often), there might be less of a need, but for quick usage, a simple macro is less boilerplate than creating a wasm-bindgen enabled struct.

samcday commented 5 years ago

I'm just of the opinion that creating ad-hoc JavaScript objects is something users may want to do.

Yeah, for sure, it's one of the first things I bumped into when I started trying to write a Web Worker purely in Rust, which is why I opened the RFC.

It's probably best to have @fitzgen weigh in here.

SGTM. If there's still appetite I'm happy to rework the RFC to propose a macro approach to easily set properties on Objects. I think the utilities to query data out of Objects would also be quite useful.

fitzgen commented 5 years ago

I don't want to set myself up as a BDFL for Gloo — I'd really prefer this is a more collective, community-driven project :)

On that note, if y'all think it would be useful to formalize some of the design proposal acceptance rules (for example, potentially make a formal team roster with required number of +1s before considering a design "accepted"), we can spin off a new issue to discuss that. This could help avoid some of the "is this something we want or not? are we making progress here? what are the next steps?"-style limbo. But also, Gloo is a very new project and I hesitate to add more bureaucracy until we're sure we want it.

Ok, so here are my actual thoughts on this issue:

I still believe that using "duck-typed interfaces" is what people should be using more often than not when they ask for "better object APIs". However, there are definitely cases where that doesn't fit (e.g. there is no loose sense of interface and we really want to work with arbitrary object properties and types of values). Therefore, it does make sense to pursue making arbitrary, untyped objects easier to work with, in my opinion. (But we probably want to prominently link to the duck-typed interfaces section of the wasm-bindgen guide and have some discussion of when you would use that instead.)
We should avoid reaching for macros as long as we can. They shouldn't be in a v1 design. We should only use those after we've exhausted how nice we can make the APIs without macros.
IntoJs seems like it is just Into<JsValue>, and I'm not convinced we need to define a new trait here.
Hiding type-mismatch panics in property getters will lead to fragile code. If the user is so sure about the types, they should probably be using duck-typed interfaces instead. At minimum the default should be to return results/options and have an unwrap_blah version that very clearly might panic.
I think we could get 99% of the utility that's on the table by providing:
- An object builder that uses some nice sprinklings of generics for defining properties.
- Some sort of getters and setters for existing object with a nice sprinking of generics (maybe extension traits for js_sys::Object; maybe a wrapper type)
And then after that is implemented and played around with a bit, I'd look at what else might be done to improve ergonomics (tweaking these APIs, adding more APIs, considering macros).

Pauan commented 5 years ago

@samcday My feeling was that if initial feedback was lukewarm then it's probably not something that should be pursued

I don't think that's a good assumption to make: I've been mostly silent because I only mentioned things when it was important to do so, and I'm still slowly digesting all of the various Gloo issues.

So I wouldn't treat silence as being rejection. Sometimes it just takes a bit more time for people to read through the issue and think about it!

Also, as @fitzgen said, we don't really have a way to "approve" ideas. So unless an idea is explicitly rejected, I think it has a fair chance.

especially considering there's approximately a trillion other useful things that could be added to Gloo right now.

It's not an either-or situation though. People will have different priorities: one person might really need WebSockets, so they push for that. Somebody else might really need the Audio API, so they might push on that...

People are not some amalgamous resource that can just be assigned to tasks, people work on what they want to work on, so that means that pushing on one task does not necessarily "take away" resources from a different task. It's not a zero sum game!

So rather than worrying about all that stuff, it's best to just focus on use cases: is this feature useful? Where? Why? What is the best way to solve those use cases?

If there is a use case for a feature, and that use case isn't already handled by another feature, then I think it's good for us to explore it, and not preemptively shut it down.

As such, I'm going to be reopening this issue.

samcday commented 5 years ago

Okay so meta-level stuff first.

if y'all think it would be useful to formalize some of the design proposal acceptance rules

Personally, yeah. For design in particular I think it's better to be a little more explicit about how the process works. I've been "sorta doing" open source for years. And by that I mean I find something interesting and do a drive-by engagement where I drop a PR or two, join a conversation here or there, and so on. It's less often that I just invite myself into a project and start driving the actual design of it. As such I'm not sure how much expectation there is for me to push a proposal forward. Especially considering the Rust ecosystem already has a rather formalized stewardship (legions of talented Mozilla engineers and other corporate sponsorship, established working group committees, etc). Put differently I was just being cautious not to march in and start stepping on other people's toes :)

Sometimes it just takes a bit more time for people to read through the issue and think about it!

Good point. I'm more accustomed to working closely in an established team, or working on my own stuff. I forgot that OSS projects often times have a different time-scale for decision making!

So unless an idea is explicitly rejected, I think it has a fair chance.

I guess what happened was I wasn't 100% confident we need a convenient Object API, and then @RReverser weighed in somewhat opposed to the idea. For me that felt like enough to think I'm probably better off working on something else.

Okay now for the issue at hand.

We should avoid reaching for macros as long as we can. They shouldn't be in a v1 design.

I can continue exploring the non-macro builder style API. My gut feel is that a macro would be better though, given that it's sort of the best of both worlds in this case: it's more expressive and it translates to static dispatches through duck-type interfaces.

I'll convert that naive benchmark suite I wrote into something a little more scientific (using Benchmark.js) and expand the patterns a little and see where the numbers end up. If it's not much different to those early results I got (where ObjectBuilder is clearly slower than all existing techniques) it feels wrong to be adding something we know is kinda slow. Though as you pointed out we can include a prominent link to the duck-type guide in the ObjectBuilder API and explain the caveats clearly.

Hiding type-mismatch panics in property getters will lead to fragile code. If the user is so sure about the types, they should probably be using duck-typed interfaces instead.

Yeah, you're right. It makes sense to return a Result<T, ?> from the ObjectProps getters. One of the things that makes object lookups a little tedious in Rust at the moment is the fact that you have to do a long call chain like this:

let val = Reflect::get(&my_obj, &JsValue::from_str("foo")).unwrap_throw().dyn_into::<MyType>().unwrap_throw();

So I think there's definitely value in providing an API that simplifies that example to this:

let val = ObjectProps::object::<MyType>(&my_obj, "foo").unwrap_throw();

All we're really doing is just collapsing the two different failure cases into one and making it generic, but that makes the user's code much more readable IMO.

RReverser commented 5 years ago

and then @RReverser weighed in somewhat opposed to the idea.

I'm sorry, I didn't mean to discourage or to look as if my suggestions have anything to do with project decisions!

It just currently seemed like there are too many different ways to do this, and not enough clear usecases to choose one, and personally in these cases I feel like it's good to experiment in separate crates outside the core, and only then merge one that is a clear winner in terms of usability for most users. Otherwise it's easy to implement something less useful in core first, and then not be able to easily break backwards compat (this is already a problem with many APIs in Rust).

That said, again, I'm not someone making any decisions :)

samcday commented 5 years ago

I'm sorry, I didn't mean to discourage ...

No need to apologize! I didn't explain that point very well. What I meant was I proposed the design but wasn't particularly in love with it myself. Having someone else weigh in with good reasons why we might not need it was enough to tip me towards scrapping it altogether. I'm easily swayed like that. Sometimes I even catch myself randomly bleating like a sheep.

Otherwise it's easy to implement something less useful in core first, and then not be able to easily break backwards compat (this is already a problem with many APIs in Rust).

Absolutely. Good API design is a balance between spending the right amount of time thinking about how to future-proof something, and never shipping anything at all out of fear that you're drawing a line in the sand in the wrong place :)

fitzgen commented 5 years ago

@samcday

I can continue exploring the non-macro builder style API. My gut feel is that a macro would be better though, given that it's sort of the best of both worlds in this case: it's more expressive and it translates to static dispatches through duck-type interfaces.

I think we should be clear about what use case we are trying to improve here.

Originally, we were talking about building arbitrary JS objects with generic getters, setters, and builders for working with arbitrary properties and arbitrarily typed values.

On the other hand, duck-typed interfaces are not for arbitrary JS objects with unconstrained sets of properties whose values can be any type. Instead, they are for when you know that you only care about a static set of properties, and you statically know those properties' types (with JsValue being "any type").

The talk of a macro emitting duck-typed interface code is surprising to me, given that my original understanding was that we were trying to address the ergonomics of working with arbitrary JS objects in this issue.

These are two very different situations that call for two very different solutions, in my opinion. I think the current ergonomics for both could be improved, but we need to have clarity about what problems we are trying to solve here.

samcday commented 5 years ago

@fitzgen

These are two very different situations that call for two very different solutions, in my opinion.

Yep, I think you nailed it here. The ideas around a macro don't need to be mutually exclusive with something like the originally proposed ObjectBuilder. I think there's room for both. I'm just going to focus on the non-macro APIs for writing arbitrary key/value pairs to an existing (or new) object, and retrieving arbitrary values from an object.

RReverser commented 5 years ago

The talk of a macro emitting duck-typed interface code is surprising to me, given that my original understanding was that we were trying to address the ergonomics of working with arbitrary JS objects in this issue.

And I got the opposite impression - that string-based property builder was just considered as a way to build arbitrary structures on the JS side, hence the alternative suggestion to make a way to create these structural objects statically, which would work better both in terms of typechecking and speed.

That's why I wrote above:

and not enough clear usecases to choose one

which sounds the same to yours

I think we should be clear about what use case we are trying to improve here.

And that's why I feel that it would be good to experiment outside the core first to identify and address these different usecases in different ways.

samcday commented 5 years ago

I've completely reworked the proposal. Everyone who previously participated (@Aloso / @Pauan / @fitzgen / @RReverser / @rylev), feel free to take a look. And to anyone else lurking, please don't hesitate to weigh in here!

Rename prospective crate to gloo-properties to more accurately reflect what it's focusing on.
Covers getting / setting properties by anything that converts to JsValue, so it supports property access with Strings, Symbols, Numbers, whatever.
Supports fluent nested object traversal. I dunno what you'd call it, but I modelled it off how I remember Java Jackson's property traversal to work. That is, it defers failure until the last operation so that you can do something like this: property(&obj, "foo").property("nested").property("real").property("deep").string(), instead of property(&obj, "foo").property("nested").unwrap_throw().property("real")....
TL;DR - really doubling down on addressing the original use case I had in mind - easy ad-hoc querying / manipulation of arbitrary JS data.

fitzgen commented 5 years ago

This new proposal is scoped much better, IMO! I think it is a great improvement.

I really like that taking &JsValue for the receiver Just Works with wasm-bindgen's deref impls.

Rename prospective crate to gloo-properties to more accurately reflect what it's focusing on.

Bike shedding: maybe name this crate "gloo-reflection", since it is explicitly targeted towards getting and setting arbitrary, untyped properties and values that presumably have some dynamic component (otherwise one would be better served by aforementioned alternative approaches).

it defers failure until the last operation

I think the idiomatic Rust thing here would be to have the methods return a Result, and then to use the ? operator as one otherwise would:

let s = property(&obj, "foo")?
    .property("nested")?
    .property("real")?
    .property("deep")?
    .string()?;

This would simplify the implementation, and would be more familiar to most Rust users.

// Setter is generic over anything that can convert into a JsValue, which is
// all core Rust types, and all web_sys types.
property(&obj, "foo").set("a direct string");
property(&obj, "foo").set(42u32);
property(&obj, "foo").set(true);

This set method seems to be missing from the API skeleton.

What is the motivation for property(&obj, "foo").property("bar").set(value) instead of property(&obj, "foo").set("bar", value)? I don't have any strong opinions here, but I (perhaps naively) would have assumed the latter API. I think the latter might be easier to implement since it doesn't require saving the property-being-set's key on the side.

    /// Finishes setting properties on target object and returns it.
    pub fn finish() -> T { /* ... */ }

Is there a reason to wait to set the properties until finish, rather than doing it as each method is defined?

If both there is not strong motivation for the delayed setting, and we want to adopt the .set(k, v) style setter above, then I think we can combine both property and setter into a single API. A sketch of such an API's usage below:

// Using an existing object:
Reflect::from(&obj)
    .property("foo")?
    .set("bar", value)?;

// Create a new object.
let obj = Reflect::new_object()
    .set(some_key, 42)?
    .set("bool", true)?
    .set("string", "cheese")?
    .done();

samcday commented 5 years ago

This new proposal is scoped much better, IMO! I think it is a great improvement.

Thanks! :tada:

Bike shedding: maybe name this crate "gloo-reflection"

Yeah, I'm really not fussed on the naming. gloo-reflection sounds better, I'll update the title to use that now. If anyone else has any other suggestions for a name feel free to put them forward.

I think the idiomatic Rust thing here would be to have the methods return a Result, and then to use the ? operator as one otherwise would:

My thought was that one may not always be able to return a Result and use the ? operator. Furthermore, the Property::property method itself doesn't do anything other than rescope what we're looking at one level deeper into an object graph, and so having it fail fast isn't really that useful since nothing happens until you call one of the getters or the set method.

Is there a reason to wait to set the properties until finish, rather than doing it as each method is defined?

I mocked up an implementation while designing this API, and I'm not deferring the property sets there (doing them as part of each set call). There was a few reasons I added the finish method:

To make it easy to create and return an object in one go, rather than having to assign the object into a variable and then pass that in to setter.
Provide a somewhat future-proofish way for us to potentially improve the performance of the Setter if we need to. For example instead of dispatching N number of js_sys::Reflect::set calls (N=number of Setter::set calls), we might be able to batch them up into a single call into a JS bridge or something For example we could be calling Object::assign instead. We could also be doing some basic string interning to avoid recreating the same property strings over and over. Initially I don't want to do any of that stuff at all and keep things simple, but having the API take ownership of the object being set and then not relinquishing it until finish is called gives us more flexibility in the future, while having (AFAICT) no adverse effect on the end-user experience.
I forgot to update the method signature, but finish should actually return -> Result<T, JsValue>. And this comes back to the earlier point about whether to fail fast or not.

I think we need to discuss the fail-fast thing a bit more. Looking at the last example you provided:

let obj = Reflect::new_object()
    .set(some_key, 42)?
    .set("bool", true)?
    .set("string", "cheese")?
    .done();

Personally I prefer this:

let obj = Reflect::new_object()
    .set(some_key, 42)
    .set("bool", true)
    .set("string", "cheese")
    .done()?;

At least, for the API examples we're talking about here, I don't see the benefit in failing fast. All it does is add extra noise to the builder chain. Internally we can be doing something like this:

struct Reflect {
  err: Option<Err<JsValue>>,
  //...
}
impl Reflect {
    fn set(self) -> Self {
        if self.err.is_some() {
            return self;
        }
        //...
    }

    fn done(self) -> Result<T, JsValue> {
        if self.err.is_some() {
            return self.err.take();
        }
        //...
    }

Looking at your example chain again, in the worst case scenario the first set fails, in which case we let a couple more short-circuited calls go through until we finally return the error in the done() call.

This set method seems to be missing from the API skeleton.

Oops, it's there now.

samcday commented 5 years ago

If both there is not strong motivation for the delayed setting, and we want to adopt the .set(k, v) style setter above, then I think we can combine both property and setter into a single API. A sketch of such an API's usage below:

I think this is an intriguing direction, but I'm not 100% sold, for a couple of reasons that my brain is too tired to articulate tonight (and I've already written a veritable of wall of text above). I'll stew on it a bit and play around with it tomorrow. If we can collapse the property() and setter() stuff into one API that would certainly be more ideal though.

Pauan commented 5 years ago

My thought was that one may not always be able to return a Result and use the ? operator.

Once try becomes stabilized it will be possible to use it:

try {
    Reflect::new_object()
        .set(some_key, 42)?
        .set("bool", true)?
        .set("string", "cheese")?
}

And in the meantime you can use a local fn or closure:

fn foo() -> Result<(), JsValue> {
    Reflect::new_object()
        .set(some_key, 42)?
        .set("bool", true)?
        .set("string", "cheese")?
}

foo()

I'm ambivalent about fail-fast vs builder, I think they're both reasonable APIs (and both styles are pervasive in Rust, so neither is "more idiomatic").

Perhaps I lean slightly more toward builder, since it is a little cleaner, and as @samcday said, it gives us a bit more flexibility with controlling when things happen.

With regard to the API:

I really don't like that Property has various methods like string, u32, u16, etc. Can't they just be replaced with a single get method?

Also, I think prop is nicer and more idiomatic than property.

I have the suspicion that it's possible to unify Property and Setter somehow, but I haven't thought too hard on it.

samcday commented 5 years ago

I really don't like that Property has various methods like string, u32, u16, etc. Can't they just be replaced with a single get method?

AFAICT, no. At least, with my limited understanding of the Rust type system. The reason set can be so simple is because we can make it generic over Into<JsValue>. However, the inverse is not true, JsValue does not implement Into<String> for example. Also, even if we did come up with a way to make a generic get method, you'd still end up having to annotate the type often anyway. For example if you just wanted to dump a property out to a log message:

log!("Thing? {}", property(&my_obj, "thing").get::<String>();

Which is worse than just having the static non-generic dispatch:

log!("Thing? {}", property(&my_obj, "thing").string();

That said, I racked my brains trying to figure out how one can do this generically and came up empty. If someone else has a clear idea of how it can be done, please weigh in, I'd love to learn how to use Rust's type system better.

samcday commented 5 years ago

Also, I think prop is nicer and more idiomatic than property.

That made me chuckle. I spent a solid 5 minutes debating in my head whether to call it prop since I prefer terse method names for readability. I figured people would hate that kind of contraction and kept it as property.

I have the suspicion that it's possible to unify Property and Setter somehow, but I haven't thought too hard on it.

Yeah I'm warming up to the idea. I'll investigate further.

samcday commented 5 years ago

Perhaps I lean slightly more toward builder, ...

I'm just going to take this quote out of context so I can further my agenda to keep the builder style :)

But seriously though, I think I came up with a better way to verbalize my thoughts on it. doing something like I/O or a sequence of operations that can fail, it makes total sense to have each step return a Result<> so you can fail fast. Especially considering the next operation may not even make sense if the previous one failed (i.e no point writing a subsequent byte if the previous write failed). In the case of a builder like this though, I view the entire builder chain as a "single" operation, and that's why I'd really prefer to only return a Result from the final done/finish/whatever_we_wanna_call_it method.

Pauan commented 5 years ago

However, the inverse is not true, JsValue does not implement Into<String> for example.

We can use JsCast for it. We'll probably need to figure out how to handle basic Rust types like String, but it's doable, even if it requires a new trait.

you'd still end up having to annotate the type often anyway

Not if the type can be inferred (and most of the time it can be).

And even in the cases where it can't be inferred, I still prefer the get::<String> version, because:

It's idiomatic. Rust doesn't use individual methods for type coercions, it uses things like into/from.
It makes it clear what the type is (and also makes it more clear that it's a simple type coercion, and not something more complex).
It works with any type which can be converted from JS, not just a small hardcoded list.

There are hundreds of JS types, we don't want to limit things!

It can work generically:

fn foo<A>(object: &JsValue) -> A where A: JsCast {
   property(object, "foo").get()
}

I figured people would hate that kind of contraction and kept it as property.

I've used languages (such as Scheme) which have a philosophy of "name things verbose and precise".

Rust is not one of those languages. It takes after C, where contractions and abbrevations are the norm. Hence Vec rather than Vector (as one example of many).

In the case of a builder like this though, I view the entire builder chain as a "single" operation

I think that's true for building an object, but I don't think it's as true for getting properties from an object (where each individual get can fail). As I said, I'm pretty ambivalent about it: both styles can work fine.

samcday commented 5 years ago

Unless I'm completely lost, JsCast will not work at all the way you're describing. JsCast is to convert between existing Javascript types (i.e casting an Object to a MyFooObject, or an Element into a CanvasElement). It is not for casting into Rust types. The trait documentation even indicates as such:

A trait for checked and unchecked casting between JS types.

My API proposal already covers returning anything that is a JsCast, see the Property::cast method.

samcday commented 5 years ago

Rust is not one of those languages. It takes after C, where contractions and abbrevations are the norm.

I don't really buy into your neat explanation of how things are named in Rust. For starters there's no mention of the balance between precise+verbose and terse+readable in the Rust API naming guidelines.

It's also quite arbitrary in the standard library. Yes, you have Vec. But you also have SystemTime. Why not SysTime? Why slice::binary_search and not slice::bin_search? Why str::to_lowercase and not str::to_lower?

I'm not gonna lose any sleep over how anything is named, it really doesn't bother me. I just disagree with your assessment that it's clear how something should or shouldn't be named ;)

I think that's true for building an object, but I don't think it's as true for getting properties from an object (where each individual get can fail).

Sure, we're in complete agreement here, and that's why the proposed API already returns a Result for get operations.

Pauan commented 5 years ago

Unless I'm completely lost, JsCast will not work at all the way you're describing.

Right, hence my "even if it requires a new trait" comment.

Ideally JsValue would implement TryInto for various types (including String and JsCast).

But failing that, we can create a new trait which basically just does that.

My API proposal already covers returning anything that is a JsCast, see the Property::cast method.

Yeah, I know, and I'm saying we should push for that (and rename it to get) rather than have a bunch of the other methods.