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

Locale with -u-fw- value other than 13 value will cause assertion #78

Closed
FrankYFTang opened this issue Oct 2, 2023 · 15 comments
Closed

Comments

@FrankYFTang
Copy link
Collaborator

This is reported by @trflynn89 in #70 (comment)

Copy over here
"Hello - I'm implementing this change over in Serenity's LibJS and had a question. Do the AO definitions of WeekdayToNumber and WeekdayToString seem backwards? Currently WeekdayToNumber returns a string, and WeekdayToString returns a number, which seems pretty confusing to me."
"
It also seems like it's possible to hit the Assert: Should not reach here. step in WeekdayToNumber if we provide an invalid value as a -u-fw extension.

For example, if the user provides en-u-fw-100, then in step 34 of Intl.Locale:

34. Let r be ! ApplyUnicodeExtensionToTag(tag, opt, relevantExtensionKeys).

Then r.[[fw]] will have a value of 100. We will pass that to WeekdayToNumber and fail that assertion.
"

@FrankYFTang
Copy link
Collaborator Author

this is a difficult issue. What should we do in this case?
Should we do in constructor throw? (we will not throw for other key type
or should we not throw in the constructor but then what should we return if user call the firstDayOfWeek ?

@FrankYFTang
Copy link
Collaborator Author

@sffc @zbraniecki @littledan @anba need advise

@sffc
Copy link
Contributor

sffc commented Oct 3, 2023

Hmm, another edge case. This is a consequence of our departure from how other subtags work based on the feedback from TG1 that we should return integers rather than strings.

Current behavior of invalid values in other subtags:

// OK: gregory1 is not a real calendar but it is BCP-47
new Intl.Locale("en", { calendar: "gregory1" }).toString()
'en-u-ca-gregory1'

// Wrong: gregory12 is too long for BCP-47
new Intl.Locale("en", { calendar: "gregory12" }).toString()
// Uncaught RangeError: Incorrect locale information provided

The following cases should always be RangeError since they are BCP-47:

  1. new Intl.Locale("en", { firstDayOfWeek: "mon1234567" }) (invalid BCP-47)
  2. new Intl.Locale("en-u-fw-mon1234567")

So, the cases we need to work out are:

  1. new Intl.Locale("en", { firstDayOfWeek: "mon123" }) (unknown weekday but valid BCP-47)
  2. new Intl.Locale("en-u-fw-mon123") (unknown weekday as BCP-47)

For each of these cases, we need to decide:

  1. What is the behavior of the constructor?
  2. What is the behavior of Intl.Locale.prototype.firstDayOfWeek?

Some options:

  1. RangeError in the constructor in both cases.
  2. Constructor succeeds; getter echoes back the string if it is not one of the well-known strings that gets converted to an integer
  3. Constructor succeeds; getter returns the default first day of week for the locale as an integer if the string is not one of the well-known strings
  4. We re-litigate the decision in Normative: Add firstDayOfWeek and consider "-u-fw" #70 and always echo back the string

I don't see a reason for cases 1 and 2 to differ in behavior.

@FrankYFTang
Copy link
Collaborator Author

  1. Constructor succeeds; getter returns undefined if the string is not one of the well-known strings

@FrankYFTang
Copy link
Collaborator Author

I don't see a reason for cases 1 and 2 to differ in behavior.

agree

@sffc
Copy link
Contributor

sffc commented Oct 13, 2023

TG2 discussion: https://github.com/tc39/ecma402/blob/master/meetings/notes-2023-10-12.md#locale-with--u-fw--value-other-than-13-value-will-cause-assertion

Conclusion: We already have a getter for the integers in getWeekInfo().firstDay, and anything we do here is going to break some consistency invariant. Therefore, TG2 is bringing a recommendation to TG1 to always return a string from Intl.Locale.prototype.firstDayOfWeek.

@FrankYFTang
Copy link
Collaborator Author

I sent a private email to @littledan to explain why we like to change Intl.Locale.prototype.firstDayOfWeek to reuturn string or undefined from number or undefined. And how Intl.Locale.prototype.getWeekInfo().firstDay already address the use case he has in mind and will not be impacted by this.

@littledan
Copy link
Member

littledan commented Oct 24, 2023

I don't want to cause you all to spend too much more time on this. Although my intuition was option 1, this isn't something that I am going to insist on.

I just don't see the argument for why this API should be lenient, given that from a developer's perspective, they have to expect it to throw. As a non-expert in this area, I would've expected that getWeekInfo().firstDay === firstDayOfWeek generally--the shape of the firstDayOfTheWeek API makes it look more like something that's directly semantically meaningful, rather than a literal fragment of the input locale string. I don't think this API is as usable as it could be.

Nevertheless, I see that the current API is consistently lenient in exactly this way: e.g., you can get hourCycle to be whatever you want as well, by passing the right string as the locale (but not as an option). Seems consistent to return a string from here, even if it's worse for JS developers. So I'm fine with ECMA-402 going that way, even if I'd prefer to go back in time and validate hc while returning a number here (calendars may be different because the set of calendars changes over time).

@FrankYFTang
Copy link
Collaborator Author

I don't want to cause you all to spend too much more time on this. Although my intuition was option 1, this isn't something that I am going to insist on.

I just don't see the argument for why this API should be lenient, given that from a developer's perspective, they have to expect it to throw. As a non-expert in this area, I would've expected that getWeekInfo().firstDay === firstDayOfWeek generally--the shape of the firstDayOfTheWeek API makes it look more like something that's directly semantically meaningful, rather than a literal fragment of the input locale string. I don't think this API is as usable as it could be.

Nevertheless, I see that the current API is consistently lenient in exactly this way: e.g., you can get hourCycle to be whatever you want as well, by passing the right string as the locale (but not as an option). Seems consistent to return a string from here, even if it's worse for JS developers. So I'm fine with ECMA-402 going that way, even if I'd prefer to go back in time and validate hc while returning a number here (calendars may be different because the set of calendars changes over time).

Dear @littledan : I think we need to look at this as two layers of functionality of the API
Layer 1 - as an API for parsing the locale string and option bag resolution layer
Layer 2- as an API to get locale information

For the API in layer 2- number is for sure what is best- and that is address in getWeekInfo().firstDay
but we still have the need for the API in layer 1 - and that is what all the getter of the Intl.Locale did- those getter are returning the parsing + option bag resolution result. And that is operate in the space of Locale string therefore, should be kept in string. Otherwise, if the string has other value, it will be very difficult to return the result. One possible case to keep it in string is the future extension to support French Revolution Calendar- which have 10 days per week:

See https://en.wikipedia.org/wiki/French_Republican_calendar#Ten_days_of_the_week

I do not think there is a strong case to support such calendar right now and I do not believe CLDR has any plan to support that. But just an example to show you in the history there are people prefer a week is not 7 days.

@littledan
Copy link
Member

Yes, I accept that this API has those two layers. I don’t think this separation does an ideal job serving its users, but it is possible to work through given the existence of the other layer. So it is OK to proceed as you proposed, from my perspective.

@FrankYFTang
Copy link
Collaborator Author

So... I plan to change the spec to

I. Intl.Locale.prototype.firstDayOfWeek returns: <<undefined, "mon", "tue", "wed", "thu", "fri", "sat", "sun">>
2. {firstDayOfWeek} for new Intl.Locale() accepts: <<undefined, "mon", "tue", "wed", "thu", "fri", "sat", "sun", "0", "1", "2", "3", "4", "5", "6", "7">> Default: undefined
where "0", "7", "sun" all repesent Sunday in option reading

@FrankYFTang
Copy link
Collaborator Author

Propose fix in #79

@FrankYFTang
Copy link
Collaborator Author

So... I plan to change the spec to

I. Intl.Locale.prototype.firstDayOfWeek returns: <<undefined, "mon", "tue", "wed", "thu", "fri", "sat", "sun">> 2. {firstDayOfWeek} for new Intl.Locale() accepts: <<undefined, "mon", "tue", "wed", "thu", "fri", "sat", "sun", "0", "1", "2", "3", "4", "5", "6", "7">> Default: undefined where "0", "7", "sun" all repesent Sunday in option reading

sorry, this is not right. I need to rework on this. Intl.Locale.prototype.firstDayOfWeek need to return string, but not just that 7 string values.

@FrankYFTang
Copy link
Collaborator Author

This issue is addressed in #79

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