feat(intl): implement Date.prototype.toLocaleString, toLocaleDateString, and toLocaleTimeString#5080
Conversation
…ng, and toLocaleTimeString
Test262 conformance changes
Fixed tests (6):Tested main commit: |
…void JS alloc in Date toLocaleString path
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5080 +/- ##
===========================================
+ Coverage 47.24% 59.32% +12.08%
===========================================
Files 476 563 +87
Lines 46892 62848 +15956
===========================================
+ Hits 22154 37285 +15131
- Misses 24738 25563 +825 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nekevss
left a comment
There was a problem hiding this comment.
Noticed a couple things while reviewing this.
core/engine/src/builtins/date/mod.rs
Outdated
| func.call(this, &[], context) | ||
| } | ||
|
|
||
| /// Returns the `[[DateValue]]` internal slot (RequireInternalSlot(dateObject, `[[DateValue]]`)). |
There was a problem hiding this comment.
nit: accessing the inner field should remain as the current style.
core/engine/src/builtins/date/mod.rs
Outdated
| // Let dateObject be the this value. | ||
| // Perform ? RequireInternalSlot(dateObject, [[DateValue]]). | ||
| // Let x be dateObject.[[DateValue]]. | ||
| let t = Self::this_date_value(this)?; |
There was a problem hiding this comment.
issue: these steps are only applicable to the intl flagged implementation.
core/engine/src/builtins/date/mod.rs
Outdated
| Err(JsError::from_opaque(JsValue::new(js_string!( | ||
| "Function Unimplemented]" | ||
| )))) | ||
| // Let dateObject be the this value. |
There was a problem hiding this comment.
nit: style, include the step numbers alongside the spec steps
| let needs_defaults = format_options.check_dtf_type(date_time_format_type); | ||
| // d. If needDefaults is true and defaults is either date or all, then | ||
| if needs_defaults && defaults != FormatDefaults::Time { | ||
| if needs_defaults && (defaults == FormatDefaults::All || defaults == FormatDefaults::Date) { |
There was a problem hiding this comment.
question: why was this changed? The previous looks correct and acts in less steps
| context: &mut Context, | ||
| ) -> JsResult<JsString> { | ||
| // FormatDateTime / PartitionDateTimePattern: TimeClip, then ToLocalTime, then format. | ||
| let x = time_clip(timestamp); |
There was a problem hiding this comment.
issue: we should not be handling this logic again.
We already handle this logic in two different places. There should not be a reason to implement it a second time. Maybe you're looking for ToLocalTime?
fa1bd92 to
755481e
Compare
|
Hi @nekevss, I've pushed updates addressing the review comments and resolved the outdated threads. Could you please take another look when you have time? Thanks! |
|
Deferring to @nekevss for the final review, but LGTM from my side |
nekevss
left a comment
There was a problem hiding this comment.
I think one last issue and then this should probably be good.
| timestamp: f64, | ||
| context: &mut Context, | ||
| ) -> JsResult<JsString> { | ||
| let time_zone_offset = match dtf.time_zone { |
There was a problem hiding this comment.
issue: keep the doc lines that are applied with the methods.
Adapting dtf can be incredibly complicated, and this function is probably going to need to be updated / reworked in order to support temporal. So keeping the comments in this instance is very important.
|
Hi @nekevss, I’ve pushed commit It keeps the spec step documentation alongside Would appreciate a re-review when you have time. |
|
Hi @nekevss, Just following up on this, I’ve addressed all the previous review comments and incorporated the requested changes. Please let me know if there’s anything else I should update or refine. Happy to iterate further. Thanks! |
|
Please don't repeatedly ping maintainers. A single request for review is enough. We'll review when we have the time |
nekevss
left a comment
There was a problem hiding this comment.
Generally looks good to me.
We'll have to keep in mind how this may need to expand in order to support Temporal in the future, but that's all TBD
This Pull Request fixes/closes #5075.
It changes the following:
Date.prototype.toLocaleString,toLocaleDateString, andtoLocaleTimeStringusingIntl.DateTimeFormatwhen theintlfeature is enabled (ECMA-402 20.4.1–20.4.3).format_date_time_localeinintl/date_time_format; exposecreate_date_time_formataspub(crate); addFormatDefaults::Alland fix default-application logic for it.this_date_valuehelper for the RequireInternalSlot /[[DateValue]]step in the three methods."Invalid Date"for NaN in all three methods; whenintlis disabled, fall back toDate.prototype.toString.#[cfg(feature = "intl")]tests: invalid receiver (TypeError), NaN →"Invalid Date",typeofchecks, locale differences (en-US vs de-DE) for all three methods, and options pipeline (dateStyle: 'short').Testing
cargo test -p boa_engine --lib -- date::tests::date_proto_to_locale_string_intl(without intl; test is skipped)cargo test -p boa_engine --features intl_bundled --lib -- date::tests::date_proto_to_locale_string_intl(with intl)