feat: locale aware sorting #2906
Conversation
✅ Deploy Preview for fakerjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #2906 +/- ##
==========================================
- Coverage 99.96% 99.95% -0.01%
==========================================
Files 2977 2977
Lines 215466 215466
Branches 950 950
==========================================
- Hits 215394 215375 -19
- Misses 72 91 +19
|
| @@ -270,7 +270,7 @@ async function updateLocaleFileHook( | |||
| console.log(`${filePath} <-> ${locale} @ ${definitionKey} -> ${entryName}`); | |||
| } | |||
|
|
|||
| return normalizeLocaleFile(filePath, definitionKey); | |||
| return normalizeLocaleFile(filePath, definitionKey, locale); | |||
There was a problem hiding this comment.
IMO we should retain the same parameter order as updateLocaleFileHook for consistency.
| let collator = null; | ||
| try { | ||
| // eslint-disable-next-line no-restricted-globals | ||
| collator = new Intl.Collator(locale.replaceAll('_', '-')); |
There was a problem hiding this comment.
Optional: IMO you could store the tranformed locale name in a separate value, then you only have to access Intl once.
const localeName = locale === base ? en : locale.replaceAllThen you theoretically dont need the catch either.
scripts/generate-locales.ts
Outdated
| // eslint-disable-next-line no-restricted-globals | ||
| collator = new Intl.Collator('en'); | ||
| } else { | ||
| throw(`Failed to create collator for locale ${locale}. Using default collator.`); |
There was a problem hiding this comment.
Please throw a proper error, adjust the message and include the cause.
ST-DDT
added
p: 1-normal
|
i want to see if the team think this approach is desirable before getting too nitpicky with this PR? |
|
I dont see any drawbacks. |
|
Possible drawbacks
|
|
Thanks for listing all the potential drawbacks.
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 I consider this a low barrier or entry as we require node for building anyway and node should always come with Intl included AFAIK. |
Draft implementation for #2905
Uses the locale key to customize the sort order.
Requires Intl
The actual changes are in
scripts/generate-locales.ts