Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GetFormatterParts failing on latest Firefox Nightly #92

Closed
marco-c opened this issue Nov 23, 2021 · 7 comments · Fixed by #97
Closed

GetFormatterParts failing on latest Firefox Nightly #92

marco-c opened this issue Nov 23, 2021 · 7 comments · Fixed by #97

Comments

@marco-c
Copy link

marco-c commented Nov 23, 2021

From https://bugzilla.mozilla.org/show_bug.cgi?id=1742572:

Standalone test case:

function GetFormatterParts(timeZone, epochMilliseconds){
  const formatter = new Intl.DateTimeFormat("en-us", {
    timeZone,
    hour12: false,
    era: "short",
    year: "numeric",
    month: "numeric",
    day: "numeric",
    hour: "numeric",
    minute: "numeric",
    second: "numeric"
  });
  const datetime = formatter.format(new Date(epochMilliseconds));
  const [date, fullYear, time] = datetime.split(/,\s+/);
  const [month, day] = date.split(" ");
  const [year, era] = fullYear.split(" ");
  const [hour, minute, second] = time.split(":");
  return {
    year: era === "BC" ? -year + 1 : +year,
    month: +month,
    day: +day,
    hour: hour === "24" ? 0 : +hour,
    minute: +minute,
    second: +second
  };
}

GetFormatterParts("America/Los_Angeles", Date.now());

GetFormatterParts expects that the formatted date-time string can be split into three parts through the regular expression /,\s+/. This worked before the update, because the formatted string looked like "11 23, 2021 AD, 05:00:00". But after the update to ICU 70, which includes an update to CLDR 40, the string now looks like "11/23/2021 A, 05:00:00", i.e. can no longer be separated into three parts when searching for a comma.

@ptomato
Copy link
Contributor

ptomato commented Nov 23, 2021

Probably we should use formatToParts() instead, this code was always iffy. We will need to figure out what this would be transpiled to on platforms that don't have formatToParts(), though.

@justingrant
Copy link
Contributor

Seems like this code could be made more tolerant of variation, which would presumably be enough to weather any possible change to the formatted output:

  • Accept 1+ non-alphanumeric characters as separators. No need to restrict to spaces.
  • Match 1-2 digits for all numeric values. Don't assume 2 digits.
  • Only look at the first character of the era, to match both "B" and "BC". Might as well make the comparison case-insensitive while we're at it.

PRs would be gratefully welcomed!

@ptomato
Copy link
Contributor

ptomato commented Nov 24, 2021

I was thinking of a change for proposal-temporal such as this one:

--- a/polyfill/lib/ecmascript.mjs
+++ b/polyfill/lib/ecmascript.mjs
@@ -2310,14 +2310,14 @@ export const ES = ObjectAssign({}, ES2020, {
   },
   GetFormatterParts: (timeZone, epochMilliseconds) => {
     const formatter = getIntlDateTimeFormatEnUsForTimeZone(timeZone);
-    // FIXME: can this use formatToParts instead?
-    const datetime = formatter.format(new Date(epochMilliseconds));
-    const [date, fullYear, time] = datetime.split(/,\s+/);
-    const [month, day] = date.split(' ');
-    const [year, era] = fullYear.split(' ');
-    const [hour, minute, second] = time.split(':');
+    const values = {};
+    const parts = formatter.formatToParts(new Date(epochMilliseconds));
+    for (const { type, value } of parts) {
+      if (type !== 'literal') values[type] = value;
+    }
+    const { era, year, month, day, hour, minute, second } = values;
     return {
-      year: era === 'BC' ? -year + 1 : +year,
+      year: era.startsWith('B') ? -year + 1 : +year, // era 'B' or 'BC'
       month: +month,
       day: +day,
       hour: hour === '24' ? 0 : +hour, // bugs.chromium.org/p/chromium/issues/detail?id=1045791

We already use formatToParts in the calendar code so maybe it shouldn't be a problem. On the other hand, it's only used for non-ISO calendars, and this would require it more generally for ISO calendar usage as well. I'm not sure what formatToParts is transpiled to for old browsers that don't support it. For the reference code in proposal-temporal I'm not concerned about old browsers (it's been generally supported since 2018 or so) but for the polyfill it might matter.

@zbraniecki
Copy link

How is the era supposed to work here? In French it is AEC and EC and in Japanese 紀元前 and 西暦.

@justingrant
Copy link
Contributor

We already use formatToParts in the calendar code so maybe it shouldn't be a problem. On the other hand, it's only used for non-ISO calendars, and this would require it more generally for ISO calendar usage as well.

@ptomato Given that we don't really need formatToParts here, I'm inclined to loosen the parsing instead of adding a new dependency and rolling the dice on polyfills. Also, I'd prefer to keep the two polyfills' code the same unless there's a really strong reason not to, and I'm not sure this counts as a "really strong reason".

How is the era supposed to work here? In French it is AEC and EC and in Japanese 紀元前 and 西暦.

@zbraniecki The formatter (obtained from getIntlDateTimeFormatEnUsForTimeZone) is always en-US.

@marco-c
Copy link
Author

marco-c commented Dec 6, 2021

@justingrant mind releasing a new version with this fix?

@justingrant
Copy link
Contributor

@12wrigja and I are planning to get a new release out to npm very soon. Hopefully this week. This fix will be included.

stekern added a commit to capralifecycle/liflig-repo-metrics that referenced this issue Feb 7, 2022
After upgrading from Node v14.18 -> v14.19, tests using the
non-production polyfill for temporal started failing. Replacing this
with an alpha version of polyfill package seems to fix the issue.

The issue seem to have been introduced due to an update to International
Components for Unicode (ICU) to v70 in Node v14.19. See this issue for
more details: js-temporal/temporal-polyfill#92
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants