tc39 / proposal-temporal

Provides standard objects and functions for working with dates and times.
https://tc39.es/proposal-temporal/docs/
Other
3.32k stars 150 forks source link

Node 12 & Node 14 `toLocaleString` can't format using the ISO calendar for PlainMonthDay/PlainYearMonth only #2107

Closed justingrant closed 1 year ago

justingrant commented 2 years ago

I saw odd behavior where tests failed in Node 12 and Node 14 when calling toLocaleString for PlainYearMonth and PlainMonthDay, but not any other Temporal types.

const ym = Temporal.PlainYearMonth.from('1976-11');
ym.toLocaleString('en', { calendar: 'iso8601'  });

The error I get is this: RangeError: cannot format PlainYearMonth with calendar iso8601 in locale with calendar gregory

What I don't understand is why I get this error in Node 12/14 but not 16/17, and why it only shows up for PlainYearMonth/PlainMonthDay but not PlainDate/PlainDateTime/etc.

I don't have a local Node 12/14 install so it's hard to debug this, but I'm hoping that someone else has a local install and can repro this!

LiviaMedeiros commented 2 years ago

Repro of calendar behaviour:

console.log(new Intl.DateTimeFormat('es-CU', { calendar: 'iso8601', timeZone: 'Cuba' }).resolvedOptions());
console.log(new Intl.DateTimeFormat('en-US', { calendar: 'persian', timeZone: 'UTC' }).resolvedOptions());
console.log(new Intl.DateTimeFormat('en-US', { calendar: 'BANANA', timeZone: 'UTC' }).resolvedOptions());
console.log(new Intl.DateTimeFormat('en-US', { calendar: 'BAD_BANANA', timeZone: 'UTC' }).resolvedOptions());

node v16.13.1:

{
  locale: 'es-CU',
  calendar: 'iso8601',
  timeZone: 'America/Havana'
}
{
  locale: 'en-US',
  calendar: 'persian',
  timeZone: 'UTC',
  era: 'narrow'
}
{
  locale: 'en-US',
  calendar: 'gregory',
  timeZone: 'UTC'
}
{
  locale: 'en-US',
  calendar: 'gregory',
  timeZone: 'UTC'
}

node v14.17.6:

{
  locale: 'es-CU',
  calendar: 'gregory', // causes this issue
  timeZone: 'America/Havana',
  fractionalSecondDigits: 0
}
{
  locale: 'en-US',
  calendar: 'persian',
  timeZone: 'UTC',
  era: 'narrow',
  fractionalSecondDigits: 0
}
{
  locale: 'en-US',
  calendar: 'gregory',
  timeZone: 'UTC',
  fractionalSecondDigits: 0
}
file:///tmp/cal.mjs:4
console.log(new Intl.DateTimeFormat('en-US', { calendar: 'BAD_BANANA', timeZone: 'UTC' }).resolvedOptions());
            ^

RangeError: Invalid calendars : BAD_BANANA
    at new DateTimeFormat (<anonymous>)
    at file:///tmp/cal.mjs:4:13
    at ModuleJob.run (internal/modules/esm/module_job.js:170:25)
    at async Loader.import (internal/modules/esm/loader.js:178:24)
    at async Object.loadESM (internal/process/esm_loader.js:68:5)

(suppressed unrelated numberingSystem/year/month/day props)

Couldn't find related changes in Node itself. Perhaps ICU update?

ptomato commented 2 years ago

I think I ran into this some time ago, but never dug deeper: https://stackoverflow.com/a/62314157/172999 (I didn't mention what JS engine I was trying that example on, but I assume it was Node 14.x)

LiviaMedeiros commented 2 years ago

iso8601 calendar works in newer versions due to workarounds in v8: v8:11295, https://github.com/v8/v8/commit/dff4f7a9210fda268b4d06d15ed56b9a3ca2ccd2, https://github.com/v8/v8/commit/a1e6efd80cc09d48016dcfe6bc8b797c15546315

ICU report the same type "gregorian" for both "gregorian" calendar and "iso8601" calendar and the same type "islamic" for both "islamic" and "islamic-rgsa" calendar. We use the alt_calendar bit to distinguish between them. When the type is "gregorian" and the alt_calendar bit is set, it is "iso8601", otherwise the true "gregorian" calendar. While the type is "islamic" and the alt_calendar bit is set, it is "islamic-rgsa" calendar, otherwise "islamic" calendar.

I.e. updating system-icu to v70.1 separately doesn't fix it for older Nodes.

ptomato commented 2 years ago

Could we fix this by just dropping support for Node 12/14 with the reference polyfill, and leave it up to production polyfills to work around the bad v8 behaviour if they choose?

ptomato commented 1 year ago

The reference polyfill already dropped support for Node 12 and 14.