-
-
Notifications
You must be signed in to change notification settings - Fork 186
fix(i18n): fallback on missing plural translation #860
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
fix(i18n): fallback on missing plural translation #860
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| // In case translation doesn't have all plural forms, use the last available form | ||
| if (plural > choicesLength - 1) { | ||
| if (import.meta.dev) { | ||
| console.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add // oxlint-disable-next-line no-console -- warn logging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Looks like we need to add a test... |
📝 WalkthroughWalkthroughThis change refactors the internationalization configuration by introducing a shared internal helper function Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/i18n.ts (1)
81-99: CacheIntl.PluralRulesinstance outside the returned closure.Creating a new
Intl.PluralRulesinstance on every pluralisation call is inefficient. Move the instantiation into the outer scope so it's created once per locale.Additionally, consider using
??instead of||on line 84 for clearer intent when handlingundefined, and typing the mapping withIntl.LdmlPluralRulekeys for stricter type safety.♻️ Proposed refactor
-function createPluralRule(locale: string, mapping: Record<string, number>) { +function createPluralRule(locale: string, mapping: Partial<Record<Intl.LdmlPluralRule, number>>) { + const pluralRules = new Intl.PluralRules(locale) return (choice: number, choicesLength: number) => { - const name = new Intl.PluralRules(locale).select(choice) - const plural = mapping[name] || 0 + const name = pluralRules.select(choice) + const plural = mapping[name] ?? 0 // In case translation doesn't have all plural forms, use the last available form if (plural > choicesLength - 1) {
| const name = new Intl.PluralRules('pl-PL').select(choice) | ||
| return { zero: 0, one: 1, two: 0, few: 2, many: 3, other: 4 }[name] | ||
| }, | ||
| pluralRule: createPluralRule('pl-PL', { zero: 0, one: 1, two: 0, few: 2, many: 3, other: 4 }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
cd /dev/shm/repo && find . -name "i18n.ts" -o -name "config" -type d | head -20Repository: npmx-dev/npmx.dev
Length of output: 124
🏁 Script executed:
cd /dev/shm/repo && fd -t f "i18n" | head -20Repository: npmx-dev/npmx.dev
Length of output: 124
🏁 Script executed:
cd /dev/shm/repo && cat -n config/i18n.ts | head -30Repository: npmx-dev/npmx.dev
Length of output: 124
🏁 Script executed:
git ls-files | head -30Repository: npmx-dev/npmx.dev
Length of output: 667
🏁 Script executed:
fd -t f "i18n.ts"Repository: npmx-dev/npmx.dev
Length of output: 75
🏁 Script executed:
cat -n config/i18n.ts | sed -n '230,310p'Repository: npmx-dev/npmx.dev
Length of output: 2197
🏁 Script executed:
cat -n config/i18n.ts | sed -n '1,100p'Repository: npmx-dev/npmx.dev
Length of output: 4818
🏁 Script executed:
grep -n "createPluralRule" config/i18n.ts | head -20Repository: npmx-dev/npmx.dev
Length of output: 758
🏁 Script executed:
node -e "console.log(new Intl.PluralRules('pl-PL').select(0))"Repository: npmx-dev/npmx.dev
Length of output: 65
🏁 Script executed:
node -e "
const rule = new Intl.PluralRules('pl-PL');
console.log('choice 0:', rule.select(0));
console.log('choice 1:', rule.select(1));
console.log('choice 5:', rule.select(5));
console.log('choice 22:', rule.select(22));
"Repository: npmx-dev/npmx.dev
Length of output: 119
Restore explicit zero handling for Polish locale or update plural form mapping.
The new implementation using createPluralRule introduces a behavioural regression. Intl.PluralRules('pl-PL').select(0) returns "many" (not "zero"), which maps to form index 3, whereas the previous implementation explicitly returned form index 0 for choice === 0. This changes how "0 items" is displayed in Polish.
Additionally, the zero: 0 key in the mapping is unreachable dead code—Intl.PluralRules('pl-PL') never returns a "zero" category for the Polish locale.
Either restore the explicit zero handling (as in the commented-out lines 241–247) within createPluralRule, or confirm this is intentional and update the mapping accordingly.

This is incomplete.Only handles Ukrainian at the moment, and for some reason the warning inside the dev if statement is not being logged.Falls back to the highest plural form if given plural amount is not translated, also logs a warning in dev mode if this happens.
This results in inaccurate plurals if translations are incomplete, but the alternative is an error page.