fix(dashboard): enforce locale-specific plural forms in translations#13268
fix(dashboard): enforce locale-specific plural forms in translations#13268radeknapora wants to merge 19 commits intomedusajs:developfrom
Conversation
…anguages Extended the i18n translation schema to support 'few' and 'many' pluralization forms alongside existing 'one' and 'other' forms. This change improves support for languages with more complex pluralization rules.
|
@radeknapora is attempting to deploy a commit to the medusajs Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Thank you for the contribution! I did not know that about slavic languages so definitely learned something today 😅. For the "required or not" question, I do not believe we have something in place to add conditions but I think it could be a good idea. So, I will leave the ball into your court if that is something you want to work on. You can tag me as a reviewer of that PR if you decide to do it! For now though, it is fine to push it like this so we at least get rid of the stripping out issue
🦋 Changeset detectedLatest commit: b65eb07 The changes in this PR will be included in the next version bump. This PR includes changesets to release 74 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Sounds good 👍 Then let’s keep it like this for now to avoid the stripping issue. I’ll look into creating a follow-up PR that introduces conditional requirements for plural forms when I get a chance. I’ll make sure to tag you as a reviewer on that one. |
if you can also add a changeset before merging! Sorry I forgot |
|
Sure, I’ve already added a changeset 👍 thanks for the reminder! |
|
Updated the changeset to follow the convention, is it okay now? |
adrien2p
left a comment
adrien2p
left a comment
There was a problem hiding this comment.
thank you for your contribution 💪 @olivermrbl how do we want to handle this one?
|
Bump @olivermrbl |
|
@radeknapora There seems to still be issues with unit tests at the moment. Any chance you can take a look at this? |
|
Hi @radeknapora :) Are you able to take a look at the unit tests for this PR? |
|
Hi @willbouch sorry I missed your earlier message. I'm currently on holiday and don’t have time to look into the unit tests right now. I’ll take a look at this PR once I’m back. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
PROBLEM WE'RE TRYING TO SOLVE Current i18n validation requires ALL languages to have identical keys. This breaks when different languages need different plural forms: - English needs: _one, _other (2 forms) - Polish needs: _one, _few, _many, _other (4 forms) - Arabic needs: _zero, _one, _two, _few, _many, _other (6 forms) When Polish adds _few/_many, tests fail because English doesn't have them. This violates i18next/CLDR best practices. PROPOSED SOLUTION (WIP) This is a working but incomplete implementation of language-aware validation: What Works: - Schema merges keys from ALL translation files (not just English) - Plural forms marked as optional in schema - Each language validated against its CLDR plural requirements - Tests pass with warnings for incomplete translations What's Missing: - NO IDE WARNINGS - VS Code won't warn about missing _few in Polish files because they're optional in the shared schema - Could generate per-language schemas ($schema-pl.json, etc.) but requires maintaining 29+ schema files CHANGES New: - scripts/i18n/plural-rules.js - CLDR plural rules for all 29 languages Modified: - scripts/i18n/generate-schema.js - Merges all languages, marks plurals optional - src/i18n/translations/__tests__/validate-translations.spec.ts - Language-aware validation - src/i18n/translations/$schema.json - Regenerated QUESTIONS FOR TEAM 1. Is this approach acceptable? (Tests pass but with warnings) 2. Should we generate per-language schemas for IDE support? 3. Better ideas for validating language-specific plural forms? Note: This unblocks Polish translations but needs team discussion on the best long-term solution.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
WIP: Proposed Solution for CLDR-Compliant i18n PluralizationStatus: Working but incomplete - seeking team feedback ProblemCurrent i18n validation requires ALL languages to have identical keys. This doesn't work because different languages have different plural rules:
When Polish translations add Proposed SolutionThis PR implements language-aware validation based on CLDR standards. What works
What's missing
Possible solutions:
Files ChangedNew:
Modified:
How It Works NowBefore (broken):
After (this PR):
Test output shows warnings for incomplete plural forms - these are informational only and don't fail the build. Questions for Team
ReferencesNote: This unblocks Polish translation work but needs team discussion on the best long-term solution for IDE validation of language-specific plural forms. |
radeknapora
changed the title
|
Hi @willbouch! I've updated the PR:
Everything should now be clearer for review. Let me know if you'd like any adjustments! |
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| "uk": ["one", "few", "many", "other"], | ||
| "vi": ["other"], | ||
| "zh": ["other"] | ||
| } |
There was a problem hiding this comment.
Bug: Missing language entries in plural configuration
The plural-config.json file is missing entries for several languages that have translation files: el, ro, mk, mn, zhCN, lt, and id. The validation script at scripts/i18n/validate-translation.js requires every language to have an entry in plural-config.json and exits with an error if not found (line 20-23). This means validation will fail for these seven languages, preventing their translation files from being validated or used.
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
@claude review this |
PR Review: Locale-Aware Pluralization ValidationThank you for this well-structured PR! The implementation of locale-specific plural form validation is a valuable addition to the i18n system. Here's my detailed review: ✅ Strengths
🔍 Code Quality Observations1. Schema Generation Logic (
|
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR was closed because it has been stalled for 5 days with no activity. |
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
Any updates on this? @NicolasGorga |
|
Thank you for your contribution! 🎉 After an initial review, this PR looks good to us. Here's a summary: ✅ PR template is complete A team member will do a final review before this is merged. We appreciate your patience! Notes:
|
| @@ -20,12 +31,28 @@ function generateSchemaFromObject(obj) { | |||
|
|
|||
| const properties = {} | |||
| const required = [] | |||
| const localPluralBaseKeys = new Set() | |||
There was a problem hiding this comment.
| const localPluralBaseKeys = new Set() | |
| const localePluralBaseKeys = new Set() |
| Object.keys(obj).forEach((key) => { | ||
| const baseKey = getBaseKey(key) | ||
| if (baseKey) { | ||
| localPluralBaseKeys.add(baseKey) | ||
| } | ||
| }) | ||
|
|
||
| Object.entries(obj).forEach(([key, value]) => { | ||
| if (getBaseKey(key)) return | ||
|
|
||
| properties[key] = generateSchemaFromObject(value) | ||
| required.push(key) | ||
| }) |
There was a problem hiding this comment.
q: why aren't we doing this in a single loop?
| localPluralBaseKeys.forEach((baseKey) => { | ||
| ALL_PLURAL_FORMS.forEach((form) => { | ||
| properties[`${baseKey}_${form}`] = { type: "string" } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
suggestion: similarly, I think this can be part of the same loop.
| "uk": ["one", "few", "many", "other"], | ||
| "vi": ["other"], | ||
| "zh": ["other"] | ||
| } |
There was a problem hiding this comment.
Add newly added languages so that the validation doesn't fail.
5 participants
Summary
What — What changes are introduced in this PR?
This PR adds locale-aware pluralization validation to the i18n system, ensuring each language uses only its grammatically correct plural forms (
zero,one,two,few,many,other) according to CLDR/i18next rules.Why — Why are these changes relevant or necessary?
Different languages require different plural forms (English needs 2, Polish needs 4, Arabic needs 6, Chinese needs 1). Without validation, translations can have:
few/many)few/many)How — How have these changes been implemented?
scripts/i18n/generate-schema.jsto detect plural base keys and auto-include all 6 CLDR forms (zero|one|two|few|many|other) as optionalscripts/i18n/validate-translation.jsto validate translations against both the schema and per-locale plural requirements usingplural-config.jsonsrc/i18n/plural-config.jsonmapping locales to their required plural formstranslations/$schema.jsonto allow all plural forms for relevant keys and adjustedrequiredlists (removing hardcodedone/otherrequirements)translations/__tests__/validate-translations.spec.tsto check required vs extra keys and assert plural forms forenmatchplural-configtranslations/en.jsonto include explicit plural keywrongQuantity_oneunderorders.fulfillment.errorTesting — How have these changes been tested, or how can the reviewer test the feature?
items_fewtoen.json) — validation should failExamples
plural-config.json structure
{ "en": ["one", "other"], "pl": ["one", "few", "many", "other"], ... }Validation behavior
Checklist
Please ensure the following before requesting a review:
yarn changesetand follow the promptsAdditional Context
References:
Note
Adds locale-aware pluralization validation and schema support, with tests and minor English translation adjustments.
scripts/i18n/generate-schema.jsto detect plural base keys and expose all forms (_zero|_one|_two|_few|_many|_other).scripts/i18n/validate-translation.jsto validate per-locale plural requirements viasrc/i18n/plural-config.jsonand Ajv schema.src/i18n/translations/$schema.jsonto include all plural forms for detected keys and relax hardcodedrequiredfor plural pairs.src/i18n/translations/__tests__/validate-translations.spec.tsto assert required vs extra keys and enforce English plural forms perplural-config.json.orders.fulfillment.error.wrongQuantity_onetoen.json.Written by Cursor Bugbot for commit b65eb07. This will update automatically on new commits. Configure here.