feat: locale aware sorting

[matthewmayer]

Draft implementation for #2905

Uses the locale key to customize the sort order.

Requires Intl

The actual changes are in scripts/generate-locales.ts

[netlify]

✅ Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	5137f05
🔍 Latest deploy log	https://app.netlify.com/sites/fakerjs/deploys/6663fe670c9c200008549cd4
😎 Deploy Preview	https://deploy-preview-2906.fakerjs.dev
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (567d66d) to head (5137f05).
Report is 2 commits behind head on next.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #2906      +/-   ##
==========================================
+ Coverage   99.95%   99.96%   +0.01%     
==========================================
  Files        2987     2987              
  Lines      216037   216038       +1     
  Branches      951      604     -347     
==========================================
+ Hits       215943   215966      +23     
+ Misses         94       72      -22     

Files	Coverage Δ
src/locales/ar/commerce/department.ts	100.00% <100.00%> (ø)
src/locales/ar/date/month.ts	100.00% <100.00%> (ø)
src/locales/ar/date/weekday.ts	100.00% <100.00%> (ø)
src/locales/ar/vehicle/manufacturer.ts	100.00% <100.00%> (ø)
src/locales/ar/vehicle/model.ts	100.00% <100.00%> (ø)
src/locales/az/color/human.ts	100.00% <100.00%> (ø)
src/locales/az/commerce/department.ts	100.00% <100.00%> (ø)
src/locales/az/commerce/product_name.ts	100.00% <100.00%> (ø)
src/locales/az/company/prefix.ts	100.00% <100.00%> (ø)
src/locales/az/date/weekday.ts	100.00% <100.00%> (ø)
... and 208 more

... and 2 files with indirect coverage changes

[matthewmayer]

i want to see if the team think this approach is desirable before getting too nitpicky with this PR?

[ST-DDT]

I dont see any drawbacks.

[matthewmayer]

Possible drawbacks

• Most other tools are not locale aware eg if I select a bunch of lines in VS code and sort them I'll get a different order
• won't work in environments with no Intl support
• changes in ICU between different versions of node might cause different sort orders
• case insensitive sorting makes it harder to spot items with inconsistent casing to other entries.

[ST-DDT]

Thanks for listing all the potential drawbacks.

• Most other tools are not locale aware eg if I select a bunch of lines in VS code and sort them I'll get a different order

True, but that is even the case without locale aware sorting as they treat upper and lowercase differently sometimes, words with suffixes are even worse due to the ' behind it that messes their order up.

I consider this a low barrier or entry as we require node for building anyway and node should always come with Intl included AFAIK.
What do the others think?

[ST-DDT]

Team Decision

• We would like to have this for v9.0
• @matthewmayer Could you please continue this PR?

[ST-DDT]

CI doesnt seem to pass. Please run pnpm run preflight.

[xDivisionByZerox]

CI doesnt seem to pass. Please run pnpm run preflight.

I did run preflight on this branch and didn't see any changes 🤔

When I switched my node version to 22 the script emitted changes, which I find very interesting TBH.

Diff

diff --git a/src/locales/lv/commerce/department.ts b/src/locales/lv/commerce/department.ts
index 605dd1c1..22af106b 100644
--- a/src/locales/lv/commerce/department.ts
+++ b/src/locales/lv/commerce/department.ts
@@ -4,9 +4,9 @@ export default [
   'Auto',
   'Bakaleja',
   'Bērnu',
+  'Datoru',
   'Dārglietu',
   'Dārzkopības',
-  'Datoru',
   'Elektronikas',
   'Filmu',
   'Grāmatu',

[matthewmayer]

i guess node 22 has a slightly different ICU version to node 20 causing a different sort order in lv. That was one of the potential drawbacks I noted in #2906 (comment)

[matthewmayer]

i'm flagging this do not merge yet. i think its quite bad if different users on different node versions end up with different generated locale files.

Although there's only one example currently, it may well be there are more examples if you run normalization on node 20+22 across all files, not just the current modules with normalization enabled (i only have node 20 installed locally at the moment, my node 22 is borked, so i cant easily test this, perhaps @xDivisionByZerox you could try?)

[matthewmayer]

Note that Node will update ICU versions even within a major Node version

https://github.com/nodejs/node/blob/v20.0.0/tools/icu/current_ver.dep - 72.1
https://github.com/nodejs/node/blob/v20.14.0/tools/icu/current_ver.dep - 75.1

[matthewmayer]

nvm use 20.0.0
node
> 'Datoru'.localeCompare('Dārglietu', 'lv')
1

nvm use 20.14.0
node
> 'Datoru'.localeCompare('Dārglietu', 'lv')
-1

[ST-DDT]

i'm flagging this do not merge yet. i think its quite bad if different users on different node versions end up with different generated locale files.

So do you think, we should generally not sort it in a locale aware manner or do you think about alternative solutions e.g. explicitly importing a specific ICU version that we do not update during major versions?

[Shinigami92]

So do you think, we should generally not sort it in a locale aware manner or do you think about alternative solutions e.g. explicitly importing a specific ICU version that we do not update during major versions?

I would like to still vote for locale aware sorting
Maybe we need to set our pipeline to node:22 for that specific check? (it is already...) so I mean, we need to format it once and set nvmrc to use node:22 or something like that

[ST-DDT]

Is there a way to download the icu stuff as a dependency?

[matthewmayer]

So do you think, we should generally not sort it in a locale aware manner or do you think about alternative solutions e.g. explicitly importing a specific ICU version that we do not update during major versions?

I would like to still vote for locale aware sorting

Maybe we need to set our pipeline to node:22 for that specific check? (it is already...) so I mean, we need to format it once and set nvmrc to use node:22 or something like that

That wouldn't be sufficient as sort order can change even within different minor versions of the same major node release.

[matthewmayer]

the more i read about it the more i feel that the ICU data is too unstable to be implicitly used in snapshot tests, and is likely to cause irritating issues for new contributors in future. See for example nodejs/node#51090 for a similar issue where changes in ICU data in a semver-minor version caused breaking tests for many projects.

A naive Unicode sort, while nonsensical for some languages, is at least stable.

[ST-DDT]

I checked the icu code and it is cpp, so we cannot just depend on a fixed version easily AFAICT.

[matthewmayer]

I'll let you discuss in next meeting. My vote would be to abandon this.

[xDivisionByZerox]

After reviewing the discussions in this PR, I would suggest to drop this feature.

A naive Unicode sort, while nonsensical for some languages, is at least stable.

I believe this is an excellent summary to the problem we face.

[ST-DDT]

Should we add a comment to the relevant code section to raise awareness of this?

[Shinigami92]

Down the rabbit-hole, I traversed the internet of a way how we could implement a stable sorting:

• https://nodejs.org/api/intl.html#providing-icu-data-at-runtime
• https://www.npmjs.com/package/icu4c-data?activeTab=versions
• https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/
• https://www.npmjs.com/package/node-icu
• https://github.com/Metrological/icudata/tree/master
• https://github.com/unicode-org/icu
• https://github.com/unicode-org/icu/releases/tag/release-75-1

However I ran out of time and maybe proceed later, or someone else can pickup the work