boa-dev / temporal

A Rust implementation of ECMAScript's Temporal API
Apache License 2.0
26 stars 4 forks source link

Implement `MonthCode`, `PartialDate`, and `Date::with` #89

Closed nekevss closed 3 months ago

nekevss commented 4 months ago

So the title basically says it all as far as implementation. That being said, there's a lot of interpretation going into this that I'd like feedback on if possible, and it could be the right direction or it could be the wrong direction (although I'm leaning the former over the latter).

The main culprit is basically PrepareTemporalFields.

Background for discussion/sanity check:

Up until recently, I basically thought it would be an implementation detail on the engine/interpreter side, but the thing that always bugged me was the requiredFields parameter, being either a List or PARTIAL. We could probably do that to specification, but we might be providing something like Some(Vec::default()) as an argument, and it basically just felt clunky.

After the recent TemporalFields update, I went to implement the TemporalFields portion of the toX abstract ops in Boa and realized that PARTIAL is never called in the toX operations, and it's actually exclusively called in with methods. We already have a sort of precedence for partials with PartialDuration.

There's some benefits to this: we can have a with method on the native rust side, ideally the complexity that exists in PrepareTemporalFields can be made a bit easier to reason about.

Potential negatives: we might end up deviating from the specification as far as the order of when errors are thrown and observability (TBD...potentially a total non-issue) and this is probably opening up a can of worms around what would be the ideal API for a PartialDate, PartialDateTime, and PartialTime.

That all being said, I think the benefits do most likely outweigh any negatives, and it would be really cool to have with method implementations. I'm just not entirely sure around the API.

Also, there's an addition of a MonthCode enum to make From<X> for TemporalFields implementations easier.

ptomato commented 4 months ago

PrepareTemporalFields has always been slightly weird. I am trying to see if it can be simplified if custom calendars are not a thing.

nekevss commented 4 months ago

I think it definitely can be split apart. Does the split between a "PartialDateLike" (so to speak) in with methods vs. a required "DateLike" JsObject in the to_temporal_x abstract make sense to the general intent of the proposal? Or am I overlooking something from those codepaths.

ptomato commented 4 months ago

Those two operations were combined into PrepareTemporalFields because in both cases, you need to do property Gets on the object (partial date, date-like property bag, or PlainDate). Without custom calendars, you don't need to do the property Gets anymore on PlainDate. But the handling of partial date vs date-like property bag is still very similar. I'm not yet sure of what is going to be the best way to simplify it.

nekevss commented 4 months ago

Yeah, the issue I'm running into is that it has to be representable via the type system, if possible, in order to implement in temporal_rs over an implementation specific method. So where the specification may have two JsObjects that can be handled differently in an abstract op because they're functionally different, we would have to treat as a separate struct in temporal_rs. Again, at least if it were to be in temporal_rs vs. a specific engine/interpreter implementation.

Honestly, assuming I'm readying the functionality correctly. This specific split might be easier to reason about than the previous PrepareTemporalFields. Or it could be off base of the intended functionality. I should probably add some unit tests 😅