Closed Bug 1912511 Opened 9 months ago Closed 16 days ago

Ship Temporal

Categories

(Core :: JavaScript: Standard Library, enhancement, P3)

Product:
Core
Component:
JavaScript: Standard Library
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
139 Branch
Tracking Flags:
Tracking Status
relnote-firefox --- 139+
firefox139 --- fixed

People

(Reporter: dminor, Assigned: dminor)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file)

Bug 1912511 - Ship temporal; r=mgaudet!
48 bytes, text/x-phabricator-request
Details | Review

The temporal champions are asking implementers to consider shipping Temporal. We should at least have a bug on file to do this.

Depends on: 1912757
Depends on: 1912782
Severity: -- → N/A
Priority: -- → P3

This can only be complete after this bug is actually fixed and Temporal landed.

Hi Anba, other than fuzzing, do you know of any bugs or spec issues that would block us from being able to ship Temporal?

Flags: needinfo?(andrebargull)

There are two open normative PRs, but both are for edge-cases and therefore shouldn't be blockers:

We should decide if we want to implement locale-sensitive Duration.prototype.toLocaleString, even though this isn't currently spec'ed. Users may expect this to work, because the polyfill already implements it:

We also likely need to add caches for time zones and calendars at some point. Possibly also cache computed calendar fields, so that for example accessing plainDate.year doesn't lead to repeatedly creating new ICU4X date and calendar objects. This probably also falls in the nice-to-have bucket, but isn't a release blocker:

I think more important are better error messages, because Temporal adds new concepts and APIs to JavaScript, which are possibly unfamiliar to developers. And cryptic error messages could prevent adaption of Temporal. I think it wouldn't hurt if the existing error messages are checked (by a native speaker) to ensure the language is correct and understandable.

Flags: needinfo?(andrebargull)
Depends on: 1839676
Assignee: nobody → dminor
Depends on: 1946823

I forgot to mention ICU4X issues.

  1. Dates too far into the past or future aren't yet supported:
  1. Islamic to ISO8601 calendar conversion bug:

Example: Temporal.PlainDate.from({year: 1390, monthCode: "M01", day: 30, calendar: "islamic"}).day returns 0.

  1. Different start of epoch for Islamic calendar:

That leads to a one day difference when Intl.DateTimeFormat, which uses ICU4C instead of ICU4X.

js> d = Temporal.Now.plainDateISO().withCalendar("islamic")
({})
js> d.month + "-" + d.day                                   
"8-14"
js> d.toLocaleString("en", {calendar:"islamic"})            
"8/15/1446 AH"

In addition to that one-day difference, ICU4X is also using different algorithms to compute dates in the Islamic and Chinese calendars. That leads to returning different results depending on whether ICU4X or ICU4C code is called:

js> d = new Temporal.PlainDate(2027, 2, 6, "chinese")
({})
js> d.month + "-" + d.day
"1-1"
js> d.toLocaleString("en", {calendar:"chinese"})
"12/30/2026"

I spoke with the ICU4X team about the ICU4X issues. They feel that the first three issues have a common cause, and there's a way ahead to fix it, which they've documented enough that it would be practical for us to fix.

For the different start of epoch for the Islamic calendar, it's not clear to them whether ICU4C or ICU4X is right, and it sounds like they're looking for more evidence, so this may take some time to resolve. I think we could carry a local patch for consistency with ICU4C without too much trouble.

I'm more concerned about the differences between calendrical algorithms. Do we think these differences are significant enough to block shipping Temporal?

If that's the case, we either have to move more code over to use ICU4X, or switch the Temporal implementation to temporarily use ICU4C calendrical code. If necessary, I think switching to ICU4C calendar code is more practical in the short term, but I'd like to hear more opinions on this.

Flags: needinfo?(hsivonen)
Flags: needinfo?(andrebargull)

When we have the ICU4X integration code already written and in general we're trying to migrate from ICU4C to ICU4X, I think it would be counter-productive to put effort into re-writing the integration code to use ICU4C instead of putting the effort into fixing the remaining ICU4X bugs.

In theory, these calendars are optional in Temporal. I suggest we test whether the optionality is really true: Let's ship Temporal with the calendars that are in a shippable state and disable for now the calendars that aren't in a shippable state. (I acknowledge that there's the issue that the spec ties the available calendars in Temporal to the available calendars in Intl.DateTimeFormat.)

As for specific issues:

  1. For dates too far back or forward, the practical impact is likely not to be significant as practical uses cases can be expected to be about near future and near past, and the ICU4X issue says ICU4C has these problems, too, but just doesn't have assertions to detect the problems. One option would be to make the binding layer have approximate limits and throw if going out of range even if that means making something up relative to the spec for now. I quick code inspection suggests the binding code already has hard-coded limits motivated by ICU4X capabilities: https://searchfox.org/mozilla-central/rev/2a69486ab1df00b1ea8ecd14027e2cc6c0415ed0/js/src/builtin/temporal/Calendar.cpp#627
  2. For the round-tripping bug, I suggest first trying to fix it and if fixing it doesn't work out within Mozilla timelines for shipping Temporal, disabling the calendar type for the initial Temporal ship.
  3. What the current day in the islamic calendar is should be a discoverable "truth on the ground" issue. We should find out and change the code accordingly, or if we cannot find out soon enough, disable the calendar for the initial Temporal ship. (If we can't establish which one of ICU4X and ICU4C matches what users of the calendar think the current day is, we shouldn't ship a potentially wrong behavior just to match ICU4C: Browsers agreeing with each other but not with the users of the calendar would not be useful.)
  4. The chinese example also seems like a "truth on the ground" fact finding issue. Do we know what the cause of the difference is? Reingold was supposed to be the correct source on these matters and ICU4X is supposed to agree with Reingold. What reason is there to believe that ICU4X is wrong and ICU4C is right for the chinese calendar here? (It's bad if Temporal and formatting don't agree, but agreeing on something that's incorrect isn't a fix.) As I understand it, dangi and chinese share logic. Does the problem also reproduce with dangi?
Flags: needinfo?(hsivonen)

(In reply to Henri Sivonen (:hsivonen) from comment #7)

  1. The chinese example also seems like a "truth on the ground" fact finding issue. Do we know what the cause of the difference is? Reingold was supposed to be the correct source on these matters and ICU4X is supposed to agree with Reingold. What reason is there to believe that ICU4X is wrong and ICU4C is right for the chinese calendar here? (It's bad if Temporal and formatting don't agree, but agreeing on something that's incorrect isn't a fix.) As I understand it, dangi and chinese share logic. Does the problem also reproduce with dangi?

Yes, this also applies to dangi.

Using https://gist.github.com/anba/4463cbf7417dd4dd7c14aa3fc60ed349 to check when ICU4X and ICU4C don't agree, it mostly(?) seems that ICU4X has the correct dates.

From the script output:

...
From 1987-07-27 to 1987-09-22 (57 days)
From 1999-01-18 to 1999-02-15 (28 days)
From 2012-08-18 to 2012-09-15 (28 days)
From 2018-11-08 to 2018-12-06 (28 days)
From 2027-02-07 to 2027-03-07 (28 days)
From 2030-02-03 to 2030-03-03 (28 days)
...

Comparing these dates with:

show that the ICU4X results are correct. ICU4X release 1.4 computed a wrong leap month in 3033, but that has been fixed in release 1.5. See also https://github.com/tc39/test262/pull/4079#issuecomment-2112078054 and https://github.com/tc39/test262/pull/4079#issuecomment-2112082687.

Furthermore when comparing ICU4X (release 1.5) against the conversion tables from https://www.hko.gov.hk/en/gts/time/conversion.htm, the only mismatch happens in 1906. ICU4X computes that the third month has 29 days and the fourth month 30 day, but it's the other way around in the HKO data. (This is all under the assumption that the HKO data match what's published by the PMO, which publish the official Chinese calendar conversions.)

Related ICU4C issues:

(In reply to Henri Sivonen (:hsivonen) from comment #7)

  1. What the current day in the islamic calendar is should be a discoverable "truth on the ground" issue. We should find out and change the code accordingly, or if we cannot find out soon enough, disable the calendar for the initial Temporal ship. (If we can't establish which one of ICU4X and ICU4C matches what users of the calendar think the current day is, we shouldn't ship a potentially wrong behavior just to match ICU4C: Browsers agreeing with each other but not with the users of the calendar would not be useful.)

At least for the Umm al-Qura calendar, ICU4C computes the same day for today as is published at https://www.ummulqura.org.sa/, i.e. February 2025 is Sha’aban 26. Whereas ICU4X computes Sha’aban 27. The actual date for the Umm al-Qura calendar can be different from the computed date when religious authorities in SA decide so according to https://webspace.science.uu.nl/~gent0113/islam/ummalqura_adjust.htm. The exact rules have changed multiple times in the last 50 years (https://webspace.science.uu.nl/~gent0113/islam/ummalqura_rules.htm), I don't know if ICU4X attempts to use older rules for past dates, or if it always uses the current rules.

I was suggesting we switch to ICU4C calendars not because I believe them to be correct, but that I thought it was better to generate consistent results rather than having different results in Firefox depending upon whether we were using Intl or Temporal APIs. To me the concerning thing is Andre's example of:

js> d = new Temporal.PlainDate(2027, 2, 6, "chinese")
({})
js> d.month + "-" + d.day
"1-1"
js> d.toLocaleString("en", {calendar:"chinese"})
"12/30/2026"

It sounds like the way ahead is to disable the Islamic, Chinese and Dangi calendars for now. I don't think we should generate inconsistent results in Firefox depending upon the API being used, even if the Temporal results are the correct ones. I'll file a bug.

Depends on: 1950425

Anba has a workaround to use ICU4X calendars in Bug 1954138 that fixes most of the inconsistencies.

Flags: needinfo?(andrebargull)

I think we can target Firefox 139 for shipping Temporal.

Status: NEW → ASSIGNED

https://hg.mozilla.org/mozilla-central/rev/ec2e2dd77335

Status: ASSIGNED → RESOLVED
Closed: 16 days ago
status-firefox139: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 139 Branch
Type: task → enhancement

Please nominate this for the Fx139 relnotes when you get a chance.

Flags: needinfo?(dminor)

Release Note Request (optional, but appreciated)
[Why is this notable]: We're shipping Temporal. Temporal is an improved version of Date, which has been a long-standing pain point in the language. We're the first browser to ship it.
[Affects Firefox for Android]:
[Suggested wording]: The Temporal proposal is now enabled by default in Firefox. Temporal is a better version of Date, for more details, please see https://spidermonkey.dev/blog/2025/04/11/shipping-temporal.html and https://tc39.es/proposal-temporal/docs/.
[Links (documentation, blog post, etc)]: https://spidermonkey.dev/blog/2025/04/11/shipping-temporal.html, https://tc39.es/proposal-temporal/docs/

relnote-firefox: --- → ?
Flags: needinfo?(dminor)

Added to the Fx139 relnotes.

relnote-firefox: ? → 139+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: