-
Notifications
You must be signed in to change notification settings - Fork 162
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
Synchronise Intl updates with latest ECMA-402 #2904
Conversation
- Update ecma262-biblio to a more recent version, but don't use the latest version (2.1.2736), because that requires replacing `IsIntegralNumber`, which has been removed upstream. - Add ecma402-biblio to replace the manual biblio entries in "spec/biblio.json".
General changes: - Synchronise with latest ECMA-402 draft, including changes from tc39/ecma402#901. - Change `CreateDateTimeFormat` to always store the hour-cycle in `dateTimeFormat.[[HourCycle]]`, so later formatting operations can select the correct pattern string. Update `Intl.DateTimeFormat.prototype.resolvedOptions` to only output "hourCycle" and "hour12" when `[[DateTimeFormat]]` actually has an `[[hour]]` field. This is needed to match the existing behaviour. - Change `Value Format Records` to prefer upper-case fields and replace `[[pattern]]` and `[[rangePatterns]]` with `[[Format]]` which holds a `DateTime Format Record`. - Correctly specify `CreateTemporalDateTime` as infallible in `HandleDateTimeTemporal{Date,MonthDay,Time}`. `CreateTemporalDateTime` in `HandleDateTimeTemporalYearMonth` is fallible, because the ISO reference day can make it an out-of-range date-time value. - Remove unreachable case for a `null` format record in `HandleDateTimeTemporal{DateTime,Instant}`. - Correct value conversion in `HandleDateTimeOthers`, because `ℤ` can't be applied on a Number value. - Update `Temporal.ZonedDateTime.prototype.toLocaleString` to accept offset time zones, now that upstream ECMA-402 supports offset time zones. - Add standalone "month" as a required format to the `[[formats]]` list. This change is needed to correctly process `Temporal.PlainYearMonth` and `Temporal.PlainMonthDay`, because right now only "year+month" and "month+day" are required formats. When the user requests a standalone "month", `BasicFormatMatcher` will compute the same penalty for "year+month" and "month+day", so both formats have the same preference. This will then result in either incorrectly using "year+month" with `Temporal.PlainMonthDay` or using "month+day" with `Temporal.PlainYearMonth`. This incorrect case can't apply when standalone "month" is a required format. - It's not possible to request that all options are supported in a standalone context, because for example "year" with "en-u-ca-islamic" will currently also add "era" to the output string in implementations. Notes: - `FormatDateTimePattern` is now an infallible operation, but adding "ins" / "del" marker tags doesn't seem to work in structured types definitions. --- Updated date-time format selection: Case 1: `dateStyle` or `timeStyle` is present. When one of the style options is present, then the previous condition > If bestFormat does not have any fields that are in Supported Fields, [...] is equivalent to testing which of `dateStyle` or `timeStyle` is `undefined`, because for example when `dateStyle` isn't `undefined`, then "year", "month", and "day" are guaranteed to be present. That also means only a single call to `DateTimeStyleFormat` is needed to compute the correct format for all types. Case 2: `dateStyle` or `timeStyle` are both absent. Add the new abstract operation `GetDateTimeFormat` which implements the same formatting options processing which is currently inlined in `CreateDateTimeFormat`. The logic is now as follows: - If at least one option from `requiredOptions` is present, then use the user inputs. - Otherwise use the default options from the `defaultOptions` list. - `requiredOptions` and `defaultOptions` have different entries based on the `required` and `defaults` arguments. - The `inherit` parameter selects which user options in addition to the members of `requiredOptions` are copied over: - When `inherit=all`, then all user inputs are copied over, even if they are unexpected in the context given by `required` and `defaults`. This is needed for backwards compatibility with the current ECMA-402 spec. For example `new Date().toLocaleTimeString("en", {era: "long"})` currently returns `"Anno Domini, 7:46:53 AM"` in all engines, which means the "era" option is used even though the formatting context is a local-time string. - When `inherit=relevant`, only relevant additional options are copied over. These are options which are relevant, but can't be elements of `requiredOptions`: - "era" can't occur in a standalone context, there it's not a member of the `requiredOptions` list. - "hourCycle" is needed for the pattern selection, see tc39/ecma402#571. - The `inherit=relevant` case only applies to `Temporal.PlainX` types to ignore unsupported options. For example `new Intl.DateTimeFormat("en", {day: "numeric"}).format(new Temporal.PlainTime())` should return the string `"12:00:00 AM"` and don't display any "day" parts. - `Temporal.Instant` and `Temporal.ZonedDateTime` support all date-time options, so no additional filtering is needed for them. - In addition to "era", the "timeZoneName" option also can't occur in a standalone context, therefore it's also not an element of the `requiredOptions` list. The default options processing in GetDateTimeFormat matches the previous Temporal draft, but it's not clear at this point if this implements previous concensus, because it means the unsupported format case can only apply when `dateStyle` or `timeStyle` are used. IOW `new Intl.DateTimeFormat("en", {day: "numeric"})` works with all `Temporal` types, including `Temporal.PlainTime`, whereas `new Intl.DateTimeFormat("en", {dateStyle: "short"})` throws when used with `Temporal.PlainTime`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't go through the second commit in detail yet, but now that we have CanonicalizeUValue in ECMA-402 would you like to include your conclusion from tc39/ecma402#828 (comment) in this PR as well?
Added b0b9467 to replace ASCII lowercase normalisation for calendar identifiers with |
@anba It's labelled "Normative", but I'm not sure it is really normative — it seems to me we are simply adjusting to a normative change that was already made upstream in ECMA-402. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this. It looks much cleaner and easier to understand now.
I don't think I fully understand the hourCycle
/hour12
issue but if you say that is what's needed to replicate existing behaviour, I believe that.
I'd consider this ready to merge, just would like to clear up your expectations for the CanonicalizeUValue commit — do you think this needs to be presented at plenary? I would say it does not.
Optionally, consider removing the resolvedOptions table and the definition of AvailableCalendars in favour of linking to the biblio versions of them. However I wouldn't want to delay the PR waiting for this, so I can also just do those later.
I don't think it's necessary to present this is at plenary. I've only marked this as "normative" because the polyfill needs to be changed, so there's at least one implementation where this change has a normative effect.
I've removed the |
Thanks. I've looked at updating the reference code to also canonicalize calendars, to match the latest ECMA-402. However, there's one test262 test which treats |
I'll also deal with AvailableCalendars in a follow up issue. |
Synchronise
Intl.DateTimeFormat
updates with latest ECMA-402, see individual commit messages for more in-depth explanations.