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

Timestamps with offset timezones throw on toLocaleString() #223

Open
zemlanin opened this issue Feb 19, 2023 · 5 comments
Open

Timestamps with offset timezones throw on toLocaleString() #223

zemlanin opened this issue Feb 19, 2023 · 5 comments

Comments

@zemlanin
Copy link

zemlanin commented Feb 19, 2023

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
Copy link
Contributor

Yep, this is a known issue over in the ECMA-402 (Internationalization) spec : tc39/ecma402#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
Copy link
Author

zemlanin commented Feb 24, 2023

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
Copy link
Contributor

justingrant commented Mar 5, 2023

@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
Copy link
Contributor

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
Copy link
Contributor

12wrigja commented Mar 6, 2023

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 :)

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

No branches or pull requests

3 participants