js-temporal / proposal-temporal-v2

Future additions to Temporal
MIT License
24 stars 1 forks source link

Better API for PlainYearMonth/PlainMonthDay toLocaleString #29

Open ptomato opened 1 year ago

ptomato commented 1 year ago

The toLocaleString() method of PlainYearMonth and PlainMonthDay usually requires some surprising extra manipulation in order for it to work. See https://github.com/tc39/proposal-temporal/issues/262#issuecomment-687281748 for the rationale. A future change might make these toLocaleString() methods easier to use.

A sampler of the copious feedback we've gotten on this part of the proposal:

Advantages:

Currently, to format a PlainYearMonth as human-readable, you have to do one of three things:

  1. Ensure the PlainYearMonth object is created with the locale's calendar

    const localeCalendar = new Intl.DateTimeFormat().resolvedOptions().calendar;
    const pym = Temporal.PlainYearMonth.from({ ...data, calendar: localeCalendar });
    console.log(pym.toLocaleString());
  2. Format into a locale whose calendar is the PlainYearMonth's calendar

    console.log(pym.toLocaleString(undefined, { calendar: pym.calendarId });
  3. Use a PlainDate instead

    console.log(date.toLocaleString(undefined, { year: 'numeric', month: 'long' });

These workarounds are surprising because they aren't necessary for other Temporal types (PlainDate, etc.) and unpleasantly verbose for what they do.

Concerns:

The original reasons for this decision still apply:

Prior art:

None yet

Constraints / corner cases:

None yet

arshaw commented 10 months ago

There's a recent ticket in temporal-polyfill's repo where someone is tripping on this confusing behavior

It seems like https://github.com/tc39/proposal-temporal/issues/2521 was pretty close to reaching an ideal solution. I favor the solution that @sffc was attempting to explain there. @justingrant was asking for more explicit description ("matching steps 1-3 above") so I'll attempt to write it:

The current behavior is sufficiently confusing that it'd be wonderful to fix it for v1, if possible.

ptomato commented 9 months ago

(Sorry, it's definitely out of scope for v1.)

sffc commented 9 months ago

Agree that this is out of scope for v1 since it is forward-compatible to change in v2. The behavior I think would be worth considering in v2 is:

  1. In Intl.DateTimeFormat.prototype.format: no changes.
    • If the object is PlainDate, PlainDateTime, or ZonedDateTime: The calendars must match OR the input calendar must be iso8601, in which case the date is converted to the formatter's calendar before formatting. Otherwise, throw an error.
    • If the object is PlainYearMonth or PlainMonthDay: The calendars must match. Otherwise, throw an error. (No special casing for iso8601 since these types cannot be automatically converted.)
  2. In Temporal.*.prototype.toLocaleString:
    • If the object is PlainDate, PlainDateTime, or ZonedDateTime: choose the calendar in the following order of priority
      1. Explicit calendar option in toLocaleString option bag (CURRENT BEHAVIOR: if the explicit calendar does not match the Temporal calendar, an exception is thrown)
      2. If the above is undefined, use the Temporal object's calendar (CURRENT BEHAVIOR: the locale's calendar must match the Temporal object's calendar)
      3. If the above is iso8601, use the locale's calendar (unchanged from current behavior)
    • If the object is PlainYearMonth or PlainMonthDay:
      1. If there is an explicit calendar option in the toLocaleString option bag, it must match the Temporal object's calendar; if not, throw an exception (CURRENT BEHAVIOR: unchanged)
      2. Otherwise, use the Temporal object's calendar for formatting (including iso8601) (CURRENT BEHAVIOR: the locale's calendar must match the Temporal object's calendar)
arshaw commented 9 months ago

@sffc, responding only to your number 2 (toLocaleString), your idea would introduce a breaking change. Please consider the following code:

Temporal.PlainDateTime.from('2024-01-01T00:00:00')
  .toLocaleString({ dateStyle: 'full', calendar: 'gregory' })

output:

arshaw commented 9 months ago

You'd need to make a "passthrough" exception for iso8601. Then there'd need to be an exception to that exception when dealing with PlainYearMonth/PlainMonthDay, since the calendars for those types need to be known upon creation and can't be overridden by the user during formatting.

So, if you retrofit your idea for toLocaleString, you get the Intl.DateTimeFormat behavior exactly.

In my opinion, the behavior for toLocaleString and Intl.DateTimeFormat should match exactly. I'm not sure why there was ever a divergence in the first place.

https://github.com/tc39/proposal-temporal/issues/2364#issuecomment-1198536529

...this is probably at this point the topic that we get the most confused people opening issues about (I found 3 at first glance), so if there's a convincingly better way to do this I'd be open to revisiting it before Stage 4, as part of the practical feedback that we get during Stage 3...

If I were to preemptively create PRs for the spec, tests, and reference polyfill myself, could this be something that's brought up in the next meeting?

ptomato commented 9 months ago

I think it would be great to create the spec, tests, and a reference polyfill here in proposal-temporal-v2. I don't see any reason not to discuss V2 stuff in the next meeting, if you want to attend.

That quote about being open to revisiting it before Stage 4 is from me, from >1.5 years ago :smile: The window for that has definitely passed. JS engines have made it clear that they need changes to the proposal to stop happening in order to move forward with implementation work, and without that implementation work we won't get to Stage 4.

sffc commented 9 months ago

@sffc, responding only to your number 2 (toLocaleString), your idea would introduce a breaking change

I tweaked my post to be more explicit about the algorithm and the differences from current behavior; does it look right now?

arshaw commented 9 months ago

@sffc, I understand. Does this pseudo-code accurately capture your proposal?:

// DateTimeFormat::format (same as v1)
// -------------------------------------------------------------------------------------------------

// for ZonedDateTime/PlainDateTime/PlainDate
function relaxed_DateTimeFormat_format(obj, formatLocale, formatOptions) {
  const internalDtf = new Intl.DateTimeFormat(formatLocale, formatOptions)
  const resolvedCalendarId = internalDtf.resolvedOptions().calendar

  if (obj.calendarId !== 'iso8601' && obj.calendarId !== resolvedCalendarId) {
    throw new Error(`A non-ISO calendar cannot be overridden by ${resolvedCalendarId}`)
  }

  return internalDtf.format(coerceToMilliseconds(obj))
}

// for PlainYearMonth/PlainMonthDay
function strict_DateTimeFormat_format(obj, formatLocale, formatOptions) {
  const internalDtf = new Intl.DateTimeFormat(formatLocale, formatOptions)
  const resolvedCalendarId = internalDtf.resolvedOptions().calendar

  if (obj.calendarId !== resolvedCalendarId) {
    throw new Error(`A ${obj.calendarId} calendar cannot be overridden by ${resolvedCalendarId}`)
  }

  return internalDtf.format(coerceToMilliseconds(obj))
}

// ::toLocaleString (v2)
// -------------------------------------------------------------------------------------------------

// for ZonedDateTime/PlainDateTime/PlainDate
function relaxed_toLocaleString(obj, formatLocale, formatOptions) {
  if (formatOptions.calendar !== undefined) {
    // use `formatOptions.calendar` as-is
  } else {
    if (obj.calendarId !== 'iso8601') {
      formatOptions.calendar = obj.calendarId
    } else {
      // `formatLocale` will determine calendar
    }
  }

  const internalDtf = new Intl.DateTimeFormat(formatLocale, formatOptions)
  return internalDtf.format(coerceToMilliseconds(obj))
}

// for PlainYearMonth/PlainMonthDay
function strict_toLocaleString(obj, formatLocale, formatOptions) {
  if (formatOptions.calendar !== undefined) {
    if (formatOptions.calendar !== obj.calendarId) {
      throw new Error(`Cannot supply a calendar that does not match ${obj.calendarId}`)
    }
  }

  formatOptions.calendar = obj.calendarId

  const internalDtf = new Intl.DateTimeFormat(formatLocale, formatOptions)
  return internalDtf.format(coerceToMilliseconds(obj))
}
arshaw commented 9 months ago

In my opinion, the proposed v2 functions derived from @sffc's explanation (hopefully faithfully transcribed to pseudocode) are the base-case solutions. Benefits:

  1. When toLocaleString is called on an iso8601-based PlainDate/PlainDateTime/ZonedDateTime, any non-iso8601 calendars from the given locale or formatting options will be honored rather than throwing an error.
  2. When toLocaleString is called for a PlainYearMonth/PlainMonthDay, the caller does not need to specify the calendar. The PlainYearMonth/PlainMonthDay's calendar will be used as the default. This allows a call to toLocaleString() with no arguments to succeed rather than error if it does not match the system's locale.

For number 2, this begs the question... if the calendar formatting option defaults to the PlainYearMonth/PlainMonthDay's calendar, and it cannot be specified as anything other than the PlainYearMonth/PlainMonthDay's calendar, should the option even exist?

I believe the same conversation was had around whether ZonedDateTime::toLocaleString would accept a timeZone formatting option, which ultimately it cannot.

A conversation would need to be had about whether v2 of Temporal should deprecate the calendar option for PlainYearMonth/PlainMonthDay::toLocaleString

Does anyone have any feedback to this proposal?

sffc commented 9 months ago

@sffc's explanation (hopefully faithfully transcribed to pseudocode)

In my proposal, toLocaleString on the three "relaxed" types never results in an exception, even if the explicit calendar does not match the receiver's calendar. The pseudocode for this would basically be to call withCalendar on the object before passing it into Intl.DateTimeFormat if the format options contain a calendar key. Also, the iso8601 is not quite what I was thinking. My revised pseudocode:

// for ZonedDateTime/PlainDateTime/PlainDate
function relaxed_toLocaleString(obj, formatLocale, formatOptions) {
  if (formatOptions.calendar !== undefined) {
    obj = obj.withCalendar(formatOptions.calendar);
  } else if (obj.calendarId !== "iso8601") {
    formatOptions.calendar = obj.calendarId;
  }

  const internalDtf = new Intl.DateTimeFormat(formatLocale, formatOptions)
  return internalDtf.format(coerceToMilliseconds(obj))
}

The strict version looks correct to me.

Does anyone have any feedback to this proposal?

Rejecting the calendar option of Temporal.*.prototype.toLocaleString may result in less surprising behavior in v2, but it is also the mechanism in v1 to prevent toLocaleString from throwing exceptions (by passing calendarId).

arshaw commented 8 months ago

Thanks for taking a look @sffc. My feedback:

In my proposal, toLocaleString on the three "relaxed" types never results in an exception, even if the explicit calendar does not match the receiver's calendar. The pseudocode for this would basically be to call withCalendar on the object before passing it into Intl.DateTimeFormat if the format options contain a calendar key.

Neither of our relaxed_toLocaleString pseudocode samples could throw errors, but I see why you included the withCalendar call, to emphasize the point that obj will assume a new calendar. However, for the sake of the pseudocode, the object's calendar is irrelevant after that point. It's only purpose is to be handed to coerceToMilliseconds, which does not care about calendar systems. It can operate purely on the ISO values to do its job. But it'd be good from a readability standpoint to elaborate on this.

Also, the iso8601 is not quite what I was thinking.

The expression obj.calendarId !== 'iso8601' operates the same in both pseudocode samples. You just collapsed my nested IF statements into an ELSEIF chain. I admit, most people find ELSEIFs more readable.

Here's the revised pseudocode:

// for ZonedDateTime/PlainDateTime/PlainDate
function relaxed_toLocaleString(obj, formatLocale, formatOptions) {
  if (formatOptions.calendar !== undefined) {
    // use the specified formatOptions.calendar as-is
    // obj will assume the provided calendar, tho coerceToMilliseconds is agnostic
    obj = obj.withCalendar(formatOptions.calendar);
  } else if (obj.calendarId !== "iso8601") {
    // otherwise,
    // if obj is opinionated about calendar (aka it's non-ISO), use it for formatting
    formatOptions.calendar = obj.calendarId;
  }
  // otherwise, formatLocale will determine the calendar for formatting

  const internalDtf = new Intl.DateTimeFormat(formatLocale, formatOptions)
  return internalDtf.format(coerceToMilliseconds(obj))
}

That should do it.

As for strict_toLocaleString (reprinted here)...

// for PlainYearMonth/PlainMonthDay
function strict_toLocaleString(obj, formatLocale, formatOptions) {
  if (formatOptions.calendar !== undefined) {
    if (formatOptions.calendar !== obj.calendarId) {
      throw new Error(`Cannot supply a calendar that does not match ${obj.calendarId}`)
    }
  }

  formatOptions.calendar = obj.calendarId

  const internalDtf = new Intl.DateTimeFormat(formatLocale, formatOptions)
  return internalDtf.format(coerceToMilliseconds(obj))
}

Rejecting the calendar option of Temporal.*.prototype.toLocaleString may result in less surprising behavior in v2, but it is also the mechanism in v1 to prevent toLocaleString from throwing exceptions (by passing calendarId).

Point taken. In V2 we should probably just deprecate toLocaleString's calendar option, not remove it.

Though this might frustrating for user that in V1, we tell them they MUST use the calendar option as a way to prevent errors, but in V2 we tell them to STOP using it because it's deprecated. Though at least things won't break.

sffc commented 8 months ago

Cool, I think this aligns with my expectations now and I acknowledge that we may have written the same functions three different times with the same behavior but different code.