boa-dev
Proposal: `ForeignObject` API (name pending)
Firstly, thanks for this amazing library, I'm using it in my own JS engine Yavashark.
I would propose an API where temporal_rs code can directly get properties from an JS object.
In my opinion, this makes sense, since there are many structs that essentially are an 1-to-1 mapping from a JS object to a struct in Rust.
In some cases, these mappings require validation (e.g PartialTime, CalendarFields), which with the API can then be moved into the temporal_rs implementation.
Additionally, it would also reduce the amount of binding code between the JS Engine and temporal_rs.
The API might look something like this:
ForeignObject trait
pub trait ForeignObject {
type Error: From<&'static str>;
type Context;
/// checks if the value is a string (e.g for things like timezone which can be a string or an object with a `timeZone` property)
fn is_string(&self, context: &mut Self::Context) -> Result<bool, Self::Error>;
/// converts the value to a string, errors if not a string
fn as_string(&self, context: &mut Self::Context) -> Result<String, Self::Error>;
/// gets a property by key, returns None if the property does not exist
fn get_number(&self, key: &str, context: &mut Self::Context) -> Result<Option<f64>, Self::Error>;
/// gets a property by key, returns None if the property does not exist
fn get_string(&self, key: &str, context: &mut Self::Context) -> Result<Option<String>, Self::Error>;
/// gets a property by key, returns None if the property does not exist
fn get_bool(&self, key: &str, context: &mut Self::Context) -> Result<Option<bool>, Self::Error>;
/// checks if the object has a property by key
fn has_key(&self, key: &str, context: &mut Self::Context) -> Result<bool, Self::Error>;
/// gets a property by key, returns None if the property does not exist (can be optimized by the implementor)
fn get_tiny_str<const LEN: usize>(
&self,
key: &str,
context: &mut Self::Context,
) -> Result<Option<TinyAsciiStr<LEN>>, Self::Error> {
match self.get_string(key, context)? {
Some(s) => {
let tiny_str = TinyAsciiStr::<LEN>::from_str(&s)
.map_err(|_| "String too long or contains non-ascii characters")?;
Ok(Some(tiny_str))
}
None => Ok(None),
}
}
/// gets a property by key, returns None if the property does not exist
fn get_u8(&self, key: &str, context: &mut Self::Context) -> Result<Option<u8>, Self::Error> {
match self.get_number(key, context)? {
Some(n) if n >= 0.0 && n <= u8::MAX as f64 && n.fract() == 0.0 => Ok(Some(n as u8)),
Some(_) => Err("Number out of range for u8".into()),
None => Ok(None),
}
}
/// gets a property by key, returns None if the property does not exist
fn get_u16(&self, key: &str, context: &mut Self::Context) -> Result<Option<u16>, Self::Error> {
match self.get_number(key, context)? {
Some(n) if n >= 0.0 && n <= u16::MAX as f64 && n.fract() == 0.0 => Ok(Some(n as u16)),
Some(_) => Err("Number out of range for u16".into()),
None => Ok(None),
}
}
/// gets a property by key, returns None if the property does not exist
fn get_i32(&self, key: &str, context: &mut Self::Context) -> Result<Option<i32>, Self::Error> {
match self.get_number(key, context)? {
Some(n) if n >= i32::MIN as f64 && n <= i32::MAX as f64 && n.fract() == 0.0 => Ok(Some(n as i32)),
Some(_) => Err("Number out of range for i32".into()),
None => Ok(None),
}
}
//... more methods for different number types as needed
}
I think this can also be used in the C API (so with v8), however it requires some more binding glue between Rust and C
Is this something that is interesting to the temporal_rs maintainers? I am happy to discuss, also on the discord.
Hey! Thanks for the proposal!
If anyone else wants to also provide their thoughts on this feel free, I'm going to write out some general thoughts.
TLDR: I'm a bit hesitant about the general idea of a trait. It could be interesting, but I can't say that anything submitted for review would be accepted.
I have entertained the thought of a binding trait multiple times over the last year and a half or so, but I typically come to the conclusion that I'm not convinced it's worth supporting. I'm just going to list out some of the reasons I've run into when thinking about this before.
First, there's been a careful line drawn between engine implementation and temporal_rs as a library, and it's been done in such a way that still makes temporal_rs valid for various implementations along with being generally available as a date/time library for the Rust ecosystem.
Second, there was a time early on in temporal_rs's development when custom calendar and time zone objects were still in the specification. There was a trait implementation then for those objects that could then be implemented for an engine's JsObject that would then effectively drill a whole into the API so the JsObject method could be called in temporal_rs. It was difficult to work with and around. It also never really felt that great. It also really blurred the line of implementation code vs. library code that I mentioned above. I was also kind of hesitant at the time of calling JS user land code from a library (but that's a different subject).
Third, the general approach that has come from the first and second points has been to provide the tools for the implementations but not overly dictate how the implementation wants to handle its integration. For instance, temporal_rs provides the primitive FiniteF64 that implements the AO's ToPositiveIntegerWithTruncation and ToIntegerWithTruncation. However, it stays away from handling the ToNumber call.
Now, that all being said, hypothetically there is a small trait that could define specifically the engine specific calls (e.g., ToNumber, ToString) and then a type that implements the integration with the trait impl. So it would be probably a lot smaller than the current one proposed. I'm also not entirely clear about the FFI considerations here though.
On one hand, I think that could be interesting, but on the other hand, I'm hesitant on whether I'd ultimately accept it, and I don't want to waste someone's time trying to figure this out without having a good idea that it'd be merged. Also, feel free to reach out on Matrix / Discord though 😄
I agree with most points, all tough, from what I read out of your message, your considerations become negligible when having the API optional and behind a feature flag.
By having it a non-default feature, you don't have any "JS-specific" code when using temporal_rs as a general rust library. Additionally it still maintains a clear line between temporal_rs and engine agnostic code (or embedder code), unless you specifically opt out of it.
My main pain point actually is slightly different. The API needs to make some assumptions about how a potential JS engine wants to call it's code. I think having some kind of context and returning a custom result should be general enough though. But the worst thing that can happen, in case that some engine is not compatible with that interface is that they need to go the "normal" route.
I actually would specifically want a trait with more than just ToNumber and ToString, since many engines might have a more packed way of storing smaller Integers. (I think boa does this too, at least for arrays). Plus in case of the TinyAsciiStr engines might have a more efficient way of converting those (e.g small string optimizations in the engine). Then you don't have to go from a stack allocated string to a heap allocated string back to a stack allocated string again.
Ultimately, it is not my decision on if an API like that would be merged, however if you say that the chance of it being accepted is relatively high, I would be willing to work on a PR implementing that.