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

AvailableCanonicalTimeZones is using a case-sensitive sort on case-insensitive data #790

Open
justingrant opened this issue Jun 7, 2023 · 7 comments

Comments

@justingrant
Copy link
Contributor

justingrant commented Jun 7, 2023

AvailableCanonicalTimeZones uses a case-sensitive sort on a list of IANA Time Zone Database identifiers. These identifiers are case-insensitive, so shouldn't the sort also be case-insensitive?

Thankfully there's only one IANA canonical time zone that is impacted by this: PST8PDT. This time zone is currently sorted before Pacific/* zones, but if it had been insensitively sorted then it'd be near the end of the list. Note that only Firefox includes the PST8PDT zone, while other implementations exclude this zone, so changing this behavior would have no impact on non-FF implementations.

function caseInsensitiveSortFunc(s1, s2) {
  s1 = s1.toLowerCase();
  s2 = s2.toLowerCase();
  if (s1 < s2) return -1;
  if (s1 > s2) return 1;
  return 0;
}

tzs = Intl.supportedValuesOf('timeZone');
tzsInsensitive = [...Intl.supportedValuesOf('timeZone')].sort(caseInsensitiveSortFunc);

console.log(`Index of PST8PDT (case sensitive): ${tzs.indexOf('PST8PDT')}`);
// => Index of PST8PDT (case sensitive): 426 
console.log(`Index of PST8PDT (case insensitive): ${tzsInsensitive.indexOf('PST8PDT')}`);
// => Index of PST8PDT (case insensitive): 466
@justingrant
Copy link
Contributor Author

@anba - because this normative change would only affect Firefox, do you have an opinion about whether this change would be a good idea or not?

@anba
Copy link
Contributor

anba commented Jun 9, 2023

I don't think it's necessary to change this. IIRC we only wanted this to be sorted to ensure the output is more deterministic. (Deterministic insofar as that consecutive calls to Intl.supportedValuesOf return the same output in the same order. And that we don't leak some implementation details to the user.)

@ben-allen
Copy link
Collaborator

@justingrant - what do you think? Move forward with this one, or close it?

@justingrant
Copy link
Contributor Author

I still think it makes sense to be consistently case-insensitive for both sorting and comparison of identifiers. Seems weird to be sensitive for one but not for the other. I admittedly don't feel that strongly though. @gibson042 @sffc what do y'all think?

@gibson042
Copy link
Contributor

I also lack strong feelings here, and since we currently have alignment between spec and implementation, my inclination is to leave the situation as-is.

@sffc
Copy link
Contributor

sffc commented Jul 11, 2023

The spec currently says:

  1. Sort result in order as if an Array of the same values had been sorted using %Array.prototype.sort% using undefined as comparefn.

We stated previously that we could choose a different ordering based on use case but that we would use the undefined sort function when lacking a clear alternative. I can dig up the notes for that if you like.

I've personally had the position for a while that the bar for not using undefined sort should be somewhat high. I think there is a use case of binary-searching the big list, and it is less error-prone to do that if they are sorted in the ECMAScript standard order.

So my position is that we should keep the current spec and document in the style guide our norms for sorting return values.

@justingrant
Copy link
Contributor Author

OK, works for me. Sounds like we'll leave the current behavior as-is.

Should I leave this open to remind folks to make the style-guide changes?

I also filed an MDN PR to warn users that time zones might be sorted unexpectedly: mdn/content#27890.

Josh-Cena added a commit to mdn/content that referenced this issue Jul 12, 2023
* Clarify sorting for time zone IDs

See tc39/ecma402#790

* Apply @Josh-Cena suggestion

Co-authored-by: Joshua Chen <sidachen2003@gmail.com>

---------

Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
VincentDev0601 added a commit to VincentDev0601/content that referenced this issue Dec 17, 2023
* Clarify sorting for time zone IDs

See tc39/ecma402#790

* Apply @Josh-Cena suggestion

Co-authored-by: Joshua Chen <sidachen2003@gmail.com>

---------

Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants