js-temporal / temporal-polyfill

Polyfill for Temporal (under construction)
ISC License
529 stars 28 forks source link

Timestamps with offset timezones throw on `toLocaleString()` #223

Open zemlanin opened 1 year ago

zemlanin commented 1 year ago

Hi. Don't know if this a polyfill issue or a spec issue. Sorry if it is the latter

Temporal.ZonedDateTime.from("2020-08-05T20:06:13[Europe/Madrid]")
  .toLocaleString(undefined, {
    day: "numeric",
    month: "short",
  });
// > "5 Aug"

Temporal.ZonedDateTime.from("2020-08-05T20:06:13[+01:00]")
  .toLocaleString(undefined, {
    day: "numeric",
    month: "short",
  });
// > RangeError: invalid time zone: +01:00
justingrant commented 1 year ago

Yep, this is a known issue over in the ECMA-402 (Internationalization) spec : https://github.com/tc39/ecma402/issues/683

This polyfill does not include any localized text, so it's unlikely that we'd polyfill this gap in Intl functionality. If we wanted to polyfill this, then we'd need to replace a lot of the guts of Intl.DateTimeFormat in order to support offset time zones with toLocaleString. Doing that would be beyond the scope of this polyfill, unless @ptomato @12wrigja @sffc can think of a practical way to polyfill it.

zemlanin commented 1 year ago

Oh, okay

For anyone having the same problem, converting ZonedDateTime to a plain one before calling toLocaleString worked for my case:

Temporal.ZonedDateTime.from("2020-08-05T20:06:13[+01:00]")
  .toPlainDateTime()
  .toLocaleString(undefined, {
    day: "numeric",
    month: "short",
  });

// > "5 Aug"
justingrant commented 1 year ago

@zemlanin Your workaround suggests how we could polyfill this, at least for some use cases. In this comment I'll sketch out a possible solution. If you wanted to PR it, we'd welcome the contribution!

  1. Export hasDateOptions and hasTimeOptions from intl.ts. Alternatively, @12wrigja do you think that we should we put all shared, exported functions into ecmascript.ts? I wasn't sure how we handle the export of polyfill-only (non-spec) utility functions elsewhere.

  2. In ZonedDateTime#toLocaleString, if either hasDateOptions or hasTimeOptions are true, and if options.timeZoneName is undefined, then the time zone won't be shown in the output. Therefore, instead of formatting this, instead we can format this.toPlainDateTime().

  3. The simple solution would simply stop at Step 2. But a somewhat-more-ornate solution could take advantage of the fact that there are IANA Etc/GMT* time zones (that Intl.DateTimeFormat will accept) for each hour of offset Etc/GMT-14 to Etc/GMT+12.

Here's a completely untested starting point for this solution:

  toLocaleString(
    locales: Params['toLocaleString'][0] = undefined,
    options: Params['toLocaleString'][1] = undefined
  ): string {
    if (!ES.IsTemporalZonedDateTime(this)) throw new TypeError('invalid receiver');

    const hasOffsetTimeZone = ES.TestTimeOfsetString(this.offset);
    if (!offsetTimeZone) return new DateTimeFormat(locales, options).format(this);

    // Pre-Temporal Intl.DateTimeFormat can't handle offset time zones, but if he time zone name isn't visible
    // then we can just format as a PlainDateTime instead and avoid this problem.
    const formatTimeZoneName = options.timeZoneName || (!hasDateOptions (options) && !hasTimeOptions(options));
    if (!formatTimeZoneName) return new DateTimeFormat(locales, options).format(this.toPlainDateTime());

    // If we get here, it's an offset time zone and we need to format the time zone name. If the offset zone is 
    // aligned on an hour boundary, we can substitute an IANA Etc/GMT* zone. 
    const { offsetNanoseconds } = this;
    if (offsetNanoseconds % 3.6e12 !== 0) {
      throw new RangeError (`Offset time zone ${this.offset} cannot be formatted with this polyfill because it is not aligned on an hour boundary`);
    }
    const hours = offsetNanoseconds / 3.6e12;
    if (hours > 14 || hours < 12) {
      throw new RangeError (`Offset time zone ${this.offset} cannot be formatted with this polyfill because it is not between -12:00 and +14:00`);
    }
    // IANA's offset zones use reverse signs for POSIX compatibility. 
    // So an ISO 8601 +05:00 is "GMT-5" in the IANA TZDB.
    const ianaOffsetZone = `GMT${hours > 0 '-' : '+'}${hours}`;
    const toFormat = this.withTimeZone(ianaOffsetZone);
    return new DateTimeFormat(locales, options).format(toFormat);
  }
justingrant commented 1 year ago

After thinking about it for a bit, I think this workaround may be better applied in intl.ts as part of the DateTimeFormat part of the polyfill, because then it would work for a wider set of use cases, and would locate the DTF-specific polyfilling together.

12wrigja commented 1 year ago

Export hasDateOptions and hasTimeOptions from intl.ts.

Wouldn't this lead to a value cycle between ecmascript.ts and intl.ts? Probably a "lazy" cycle nonetheless, but that's maybe still a problem.

Leaving all that logic within intl.ts sounds reasonable to me.

I'm happy to review a PR for this for "typescript correctness", but not for business logic correctness :)