feat(temporal): Limited support for Temporal#1416
feat(temporal): Limited support for Temporal#1416nabetti1720 wants to merge 4 commits intoawslabs:mainfrom
Conversation
b4c9989 to
000ee83
Compare
b1505fd to
3ec136d
Compare
3ec136d to
6b6484e
Compare
|
@richarddavison Please take a look when you have time. :) |
|
The choice of jiff is interesting vs chrono or time (which we probably already have in the tree) |
I really like Here is some interesting documentation about
|
|
@Sytten It would be great to optimize further if RquickJS supported seamless BigInt<->i128 conversion. Is there a good way to go about this? |
|
Make sense! I will have to check it, it is still niche for now so limited support in other libs. Not a fan that it has not seen a release in 1y+ As for BigInt the C API is limited an f64 as far as O remember. You can try to add an API in rquickjs but it will likely need to be added on the C side (guys are pretty responsive though). |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
We've added a few more supportable properties to ZonedDateTime. And that's really it. |
richarddavison
left a comment
There was a problem hiding this comment.
Nice! Thanks for the PR, some minor comments and questions to address other wise looks good!
libs/llrt_utils/src/result.rs
Outdated
| self.map_err(|err| Exception::throw_message(ctx, &err.to_string())) | ||
| } | ||
|
|
||
| fn map_err_range(self, ctx: &Ctx) -> Result<T> { |
There was a problem hiding this comment.
or_throw_range maybe for consistency?
There was a problem hiding this comment.
or_throw() is a method that unconditionally specifies the msg of the caller, but I wanted a shorthand equivalent for other error types.
Ideally, we would haveor_throw_msg (alias as or_throw), or_throw_type and or_throw_range, but they are already defined.
pub trait ResultExt<T> {
fn or_throw_msg(self, ctx: &Ctx, msg: &str) -> Result<T>;
fn or_throw_range(self, ctx: &Ctx, msg: &str) -> Result<T>;
fn or_throw_type(self, ctx: &Ctx, msg: &str) -> Result<T>;
fn or_throw(self, ctx: &Ctx) -> Result<T>;
}I'd like to refactor it to the following, what do you think?
pub enum By {
Internal,
Message,
Range,
Reference,
Syntax,
Type,
}
pub trait ResultExt<T> {
fn or_throw_msg_into_msg(self, ctx: &Ctx, msg: &str) -> Result<T>;
fn or_throw_range_into_msg(self, ctx: &Ctx, msg: &str) -> Result<T>;
fn or_throw_type_into_msg(self, ctx: &Ctx, msg: &str) -> Result<T>;
fn or_throw(self, ctx: &Ctx) -> Result<T>;
fn or_throw_by(self, ctx: &Ctx, by: By) -> Result<T>;
}Since it is used so frequently, I think it is necessary to aim for a description method that is as short as possible.
There was a problem hiding this comment.
I still don't get why we can't use the regular :
fn or_throw_msg(self, ctx: &Ctx, msg: &str) -> Result<T>;
fn or_throw_range(self, ctx: &Ctx, msg: &str) -> Result<T>;
fn or_throw_type(self, ctx: &Ctx, msg: &str) -> Result<T>;
fn or_throw(self, ctx: &Ctx) -> Result<T>;What's missing? Why do we need the additional methods?
There was a problem hiding this comment.
fn or_throw(self, ctx: &Ctx) -> Result<T> {
self.map_err(|err| Exception::throw_message(ctx, &err.to_string()))
}We need a shorthand method like or_throw() that will throw another Exception while still printing the internal message of the error object.
fn or_throw_by(self, ctx: &Ctx, by: By) -> Result<T> {
match by {
By::Internal => self.map_err(|err| Exception::throw_internal(ctx, &err.to_string())),
By::Message => self.map_err(|err| Exception::throw_message(ctx, &err.to_string())),
By::Range => self.map_err(|err| Exception::throw_range(ctx, &err.to_string())),
By::Reference => self.map_err(|err| Exception::throw_reference(ctx, &err.to_string())),
By::Syntax => self.map_err(|err| Exception::throw_syntax(ctx, &err.to_string())),
By::Type => self.map_err(|err| Exception::throw_type(ctx, &err.to_string())),
}
}There was a problem hiding this comment.
or_throw_range(&ctx, "") or or_throw_type(&ctx, "") would also work. It's a bit hacky, but I'll fix it if you prefer.
EDIT: This is a bit off topic, but jiff's error messages are very strict and clear, so there's no reason not to take advantage of them.
% cat reproduction.js
const zdt = Temporal.ZonedDateTime.from("2021-07-01T12:00:00-05:00[America/New_York]");
console.log(zdt);
% llrt reproduction.js
RangeError: datetime could not resolve to a timestamp since `reject` conflict resolution was chosen, and because datetime has offset `-05`, but the time zone `America/New_York` for the given datetime unambiguously has offset `-04`
at <anonymous> (/Users/shinya/Workspaces/llrt-test/reproduction.js:1:35)
modules/llrt_temporal/src/instant.rs
Outdated
| if let Some(str) = info.as_string().and_then(|s| s.to_string().ok()) { | ||
| return Self::from_str(&ctx, &str); | ||
| } |
There was a problem hiding this comment.
Can we borrow the JS string to avoid to_string call?
There was a problem hiding this comment.
I looked at other use cases for LLRT, but I couldn't find anything other than converting to the (Standard) String type using as_string() -> to_string(). Is there any good way to do this?
There was a problem hiding this comment.
I found an implementation like this, but does it fit your needs?
llrt/libs/llrt_json/src/stringify.rs
Lines 291 to 303 in 71807b9
The rquickjs itself has a similar implementation, so I'm not sure how much of an advantage there is in using methods other than to_string().
rquickjs-core/src/value/string.rs:
/// Convert the JavaScript string to a Rust string.
pub fn to_string(&self) -> Result<StdString> {
let mut len = mem::MaybeUninit::uninit();
let ptr = unsafe {
qjs::JS_ToCStringLen(self.0.ctx.as_ptr(), len.as_mut_ptr(), self.0.as_js_value())
};
if ptr.is_null() {
// Might not ever happen but I am not 100% sure
// so just incase check it.
return Err(Error::Unknown);
}
let len = unsafe { len.assume_init() };
let bytes: &[u8] = unsafe { slice::from_raw_parts(ptr as _, len as _) };
let result = str::from_utf8(bytes).map(|s| s.into());
unsafe { qjs::JS_FreeCString(self.0.ctx.as_ptr(), ptr) };
Ok(result?)
}| fn value_of(&self, ctx: Ctx<'_>) -> Result<()> { | ||
| Err(Exception::throw_type( | ||
| &ctx, | ||
| "can't convert Instant to primitive type", | ||
| )) | ||
| } |
There was a problem hiding this comment.
why do we have a method that just throws?
There was a problem hiding this comment.
It's a requirement specification.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal/Instant/valueOf
Because both primitive conversion and number conversion call valueOf() before toString(), if valueOf() is absent, then an expression like instant1 > instant2 would implicitly compare them as strings, which may have unexpected results. By throwing a TypeError, Temporal.Instant instances prevent such implicit conversions. You need to explicitly convert them to numbers using Temporal.Instant.prototype.epochNanoseconds, or use the Temporal.Instant.compare() static method to compare them.
ab0330d to
36790d7
Compare
36790d7 to
8cb8055
Compare
|
@richarddavison I think I've fixed most of the issues, but I'd like some further feedback on some of them. :) |
Issue # (if available)
n/a
Description of changes
jiff, andllrt_temporalis merely a very thin wrapper to conform to the Temporal specification.In the future, we may consider usingtemporal_rsto have calendar functionality. However, it would be even better if we could switch to it in the backend for use cases that don't require a calendar, but that's not the goal of this PR.icuandjiff-icu(but not in this PR). If we do, we may place them after the feature gate due to the increased footprint.https://github.com/BurntSushi/jiff/blob/master/COMPARE.md#icu-v150
Not included in this PR (postponed to next time)
Temporal.Instant.round()Temporal.Duration.round()Temporal.Duration.total()Temporal.ZoneDateTime.round()Intl.DateTimeFormat()Checklist
tests/unitand/or in Rust for my feature if neededmake fixto format JS and apply Clippy auto fixesmake checktypes/directoryBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.