ecma402 icon indicating copy to clipboard operation
ecma402 copied to clipboard

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

Open justingrant opened this issue 2 years ago • 12 comments

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 avatar Jun 07 '23 04:06 justingrant

@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?

justingrant avatar Jun 07 '23 18:06 justingrant

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

anba avatar Jun 09 '23 09:06 anba

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

ben-allen avatar Jul 05 '23 22:07 ben-allen

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?

justingrant avatar Jul 10 '23 20:07 justingrant

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.

gibson042 avatar Jul 10 '23 21:07 gibson042

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.

sffc avatar Jul 11 '23 11:07 sffc

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: https://github.com/mdn/content/pull/27890.

justingrant avatar Jul 11 '23 21:07 justingrant

Style guide updated by https://github.com/tc39/ecma402/pull/831

ben-allen avatar Aug 17 '24 14:08 ben-allen

The underlying problem should no longer occur because PST8PDT is now removed from the list of canonical time zones in IANA and CLDR.

I still think it would be sensible to use a case-insensitive sort instead of lexicographic for case-insensitive string data, but perhaps it's OK to wait to have that argument until there's a time that it will actually matter for observable results.

justingrant avatar Aug 18 '24 20:08 justingrant

I share that preference, and once removal of PST8PDT has fully propagated, we'll be in a position where the change from case-sensitive sorting to case-insensitive is unobservable, at which point I would feel comfortable making it. My primary motivation above for leaving things alone was avoiding visible churn, which will soon be a non-issue thanks to your efforts.

gibson042 avatar Aug 19 '24 16:08 gibson042

reopening based on @gibson042 and @justingrant comments, tagging as blocked until such time as PST8PDT removal propagates

ben-allen avatar Aug 19 '24 19:08 ben-allen

Cool. If we make this change, then we should also amend the style guidelines too.

justingrant avatar Aug 19 '24 21:08 justingrant