chore: update compare-translations.ts logic#1063
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughprepare-json-files.ts: adds exported mergeLocaleObject(locale: LocaleObject, copy = false) that returns merged object or handles copy-only flows; prepareJsonFiles now explicitly returns Promise; mergeLocale calls mergeLocaleObject and writes the resulting source when present; internal NestedObject alias added and helpers loadJsonFile and getFileName were moved and remain non-exported. scripts/compare-translations.ts: refactored to async/ESM, introduces LocaleInfo and per-locale maps, adds extractLocalInfo, populateLocaleCountries, checkCountryVariant and checkJsonName, makes loadJson async (optionally using mergeLocaleObject), converts processLocale/run* to async, and omits empty nested objects during sync/fix. 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lunaria/prepare-json-files.ts (2)
40-61:⚠️ Potential issue | 🔴 Critical
mergeLocaleObjectreturnsundefinedfor single-file locales — root cause of the pipeline failure.Two issues in this function:
- Returns
undefinedfor single-file locales (lines 44–46): The function copies the file as a side effect but returns nothing, which causes theTypeError [ERR_INVALID_ARG_TYPE]crash inmergeLocale(line 73) and will also breakloadJsonincompare-translations.ts(line 110).fs.cpside effect (line 45): A function namedmergeLocaleObjectthat's exported as a pure data utility should not perform file-system writes. The copy belongs in the wrappermergeLocale.🐛 Proposed fix — return parsed JSON for all paths; move copy to mergeLocale
export async function mergeLocaleObject(locale: LocaleObject) { const files = locale.files ?? [] if (locale.file || files.length === 1) { const json = locale.file ?? (files[0] ? getFileName(files[0]) : undefined) if (!json) return - await fs.cp(path.resolve(`${localesFolder}/${json}`), path.resolve(`${destFolder}/${json}`)) - return + return await loadJsonFile(json) } const firstFile = files[0] if (!firstFile) return const source = await loadJsonFile(getFileName(firstFile)) let currentSource: unknown for (let i = 1; i < files.length; i++) { const file = files[i] if (!file) continue currentSource = await loadJsonFile(getFileName(file)) deepCopy(currentSource, source) } return source }
71-77:⚠️ Potential issue | 🔴 Critical
mergeLocaledoes not guard againstundefinedfrommergeLocaleObject.Even after fixing the upstream function, early-return paths (e.g. no files at all) can still yield
undefined.JSON.stringify(undefined)produces the JS valueundefined, whichfs.writeFilerejects withERR_INVALID_ARG_TYPE. Add a guard here.🛡️ Proposed fix
async function mergeLocale(locale: LocaleObject) { const source = await mergeLocaleObject(locale) + if (source === undefined) return await fs.writeFile( path.resolve(`${destFolder}/${locale.code}.json`), JSON.stringify(source, null, 2), ) }
🧹 Nitpick comments (3)
scripts/compare-translations.ts (3)
33-41: Unchecked array destructuring fromsplit.
locale.split('-')returnsstring[], so bothlangandcountryarestring | undefinedat runtime (depending onnoUncheckedIndexedAccess).langis assigned toLocaleInfo.lang: stringwithout a guard. Whilesplitalways returns at least one element in practice, the coding guidelines require explicit checks when accessing array values by index.🛡️ Proposed fix
const extractLocalInfo = ( filePath: string, forCountry: boolean = false, mergeLocale: boolean = false, ): LocaleInfo => { const locale = basename(filePath, '.json') - const [lang, country] = locale.split('-') - return { filePath, locale, lang, country, forCountry, mergeLocale } + const parts = locale.split('-') + const lang = parts[0] + if (!lang) { + throw new Error(`Invalid locale file name: "${locale}"`) + } + const country = parts[1] + return { filePath, locale, lang, country, forCountry, mergeLocale } }As per coding guidelines,
**/*.{ts,tsx,vue}: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
138-144:delete result[key]is a no-op —keywas never assigned toresult.At this point in the loop,
result[key]has not yet been set, sodelete result[key]does nothing. Acontinue(or simply not assigning) would express the intent more clearly.♻️ Suggested simplification
const data = syncLocaleData(refValue, nextTarget, stats, fix, propertyPath) // don't add empty objects: --fix will prevent this - if (Object.keys(data).length === 0) { - delete result[key] - } else { + if (Object.keys(data).length > 0) { result[key] = data }
330-342:populateLocaleCountries()is called after loading the reference — consider calling it earlier.Currently the reference content is loaded at line 332 via
loadJson, which internally callscheckJsonName→checkCountryVariant→ reads fromcontries. Because the referenceLocaleInfohasmergeLocale: undefined(falsy), it bypasses the merge path and thecontrieslookup doesn't affect it. However, this ordering is fragile — if the reference loading ever switches to the merge path, thecontriesmap will be empty.Moving
populateLocaleCountries()beforeloadJsonwould make the initialisation order explicit and safe.♻️ Suggested reordering
const run = async (): Promise<void> => { const referenceFilePath = join(LOCALES_DIRECTORY, REFERENCE_FILE_NAME) + + populateLocaleCountries() + const referenceContent = await loadJson({ filePath: referenceFilePath, locale: 'en', lang: 'en', }) const args = process.argv.slice(2) const fix = args.includes('--fix') const targetLocale = args.find(arg => !arg.startsWith('--')) - populateLocaleCountries() - if (targetLocale) {
|
somethings wrong with |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
lunaria/prepare-json-files.ts (2)
40-67: Implicit return type isPromise<void | unknown>— consider narrowing.
mergeLocaleObjectcan returnundefined(lines 46, 49), a parsed JSON object (line 51), or the mergedsource(line 66). The callers incompare-translations.tscast the result withas NestedObject. An explicit return type (e.g.Promise<Record<string, unknown> | undefined>) would make the contract clearer and help catch misuse at compile time.
69-71:loadJsonFilereturnsanyfromJSON.parse.This is a minor type-safety gap. Returning
unknown(orRecord<string, unknown>) instead of the implicitanywould propagate stricter checks to all callers.♻️ Proposed fix
-async function loadJsonFile(name: string) { - return JSON.parse(await fs.readFile(path.resolve(`${localesFolder}/${name}`), 'utf8')) +async function loadJsonFile(name: string): Promise<Record<string, unknown>> { + return JSON.parse(await fs.readFile(path.resolve(`${localesFolder}/${name}`), 'utf8')) as Record<string, unknown> }As per coding guidelines,
**/*.{ts,tsx,vue}: "Ensure you write strictly type-safe code".scripts/compare-translations.ts (4)
43-58:extractLocalInfois called with locale codes, not file paths.Lines 51–54 pass bare strings like
"en"or"en-US"as thefilePathargument toextractLocalInfo. ThefilePathfield in the resultingLocaleInfothen holds a locale code rather than an actual path. This doesn't cause a runtime bug today becausefilePathfrom these map entries is never read, but it's semantically misleading and fragile if anyone later relies oncontriesentries having a realfilePath.Consider either renaming the parameter / field to something like
localeOrPath, or passing a proper path.
150-156:delete result[key]on line 153 is a no-op — simplify the branch.At this point
result[key]has never been assigned, sodelete result[key]does nothing. The intent (skip empty nested objects) is already achieved by only assigning in theelsebranch. Simplifying removes dead code and makes the intent clearer.♻️ Proposed fix
const data = syncLocaleData(refValue, nextTarget, stats, fix, propertyPath) // don't add empty objects: --fix will prevent this - if (Object.keys(data).length === 0) { - delete result[key] - } else { + if (Object.keys(data).length > 0) { result[key] = data }
288-288: Prefer optional chaining over!assertion onresults[0].The ternary guard ensures
results.length > 0, but optional chaining is more idiomatic and equally concise:♻️ Proposed fix
- const referenceKeysCount = results.length > 0 ? results[0]!.referenceKeys.length : 0 + const referenceKeysCount = results[0]?.referenceKeys.length ?? 0As per coding guidelines,
**/*.{ts,tsx,vue}: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
342-361: Reference localeLocaleInfohard-codeslocale: 'en'— fragile coupling.The reference file is
en.json(line 10,REFERENCE_FILE_NAME), but theLocaleInfoat line 347–348 hard-codeslocale: 'en'andlang: 'en'independently. IfREFERENCE_FILE_NAMEever changes, these literals would silently drift. Consider deriving both values from the constant:const referenceLocale = basename(REFERENCE_FILE_NAME, '.json')
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lunaria/prepare-json-files.ts (2)
40-43: Return typePromise<void | unknown>collapses toPromise<unknown>— consider a narrower signature.
void | unknownis equivalent tounknownin TypeScript's type system, so thevoidconveys no extra compile-time safety. Callers (likemergeLocale) rely on a falsy check to distinguish "no result" from "has data", which works at runtime but isn't enforced by the type.A more descriptive return type would make the contract clearer:
♻️ Suggested improvement
export async function mergeLocaleObject( locale: LocaleObject, copy = false, -): Promise<void | unknown> { +): Promise<Record<string, unknown> | undefined> {This would require a small cast/assertion when returning the result of
loadJsonFile/ buildingsource, but it gives callers proper type narrowing withif (source).
80-91:mergeLocalesilently succeeds whenmergeLocaleObjectreturns a falsy-but-valid value.The guard
if (!source)on Line 82 would also skip writing ifsourcehappened to be an empty object{}— wait, no,!{}isfalse, so an empty object would pass. The actual concern is that any falsy value (e.g.0,"",false) returned frommergeLocaleObjectwould be skipped. In practice this won't happen because JSON locale files always parse to objects, but a=== undefinedcheck would be more precise and intention-revealing.♻️ Suggested tightening
const source = await mergeLocaleObject(locale, true) - if (!source) { + if (source === undefined) { return }
|
I think there are two issues.
> pnpm i18n:check:fix
> npmx@0.0.0 i18n:check:fix /home/shuuji3/dev/npmx.dev
> node scripts/compare-translations.ts --fix
Error: Invalid locale file "az", it should be included at "countryLocaleVariants" in config/i18n.ts, or change the name to include country name "az-<country-name>"
ELIFECYCLE Command failed with exit code 1.
en.json {
"parent1": "parent1 from en.json",
"parent2": {
"child1": "child1 from en.json",
"child2": "child2 from en.json"
}
}es.json {
"parent1": "parent1 from es.json",
"parent2": {
"child1": "child1 from es.json"
}
}es-419.json {
"parent2": {
"child2": "child2 from es-419.json"
}
}> pnpm i18n:check:fix
> npmx@0.0.0 i18n:check:fix /home/shuuji3/dev/npmx.dev
> node scripts/compare-translations.ts --fix
=== Translation Audit (with --fix) ===
Reference: en.json (3 keys)
Checking 23 locale(s)...
--- es.json ---
ADDED MISSING KEYS (with EN placeholder)
- parent2.child2
=== Summary ===
Added missing keys (EN placeholder): 1and it modified like this: locales/es.json {
"parent1": "parent1 from es.json",
"parent2": {
"child1": "child1 from es.json",
"child2": "EN TEXT TO REPLACE: child2 from en.json"
}
}locales/es-419.json {
"parent1": "parent1 from es.json",
"parent2": {
"child1": "child1 from es.json",
"child2": "child2 from es-419.json"
}
}The first |
|
I thought I also tried > pnpm build:lunaria
> npmx@0.0.0 build:lunaria /home/shuuji3/dev/npmx.dev
> node ./lunaria/lunaria.ts
ERROR The file lunaria/files/en-US.json is being tracked but no commits have been found. Ensure all tracked files in your working branch are committed before running Lunaria. 1:27:33 AM
ELIFECYCLE Command failed with exit code 1.It seems that some script removed a newline at EOF only for these five files under 
|
|
|
|
@shuuji3 I need to merge main, I fixed |
You don't need to change Right now if you run |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/compare-translations.ts (1)
194-217:⚠️ Potential issue | 🟠 MajorCountry variant files will be overwritten with merged (bloated) content during
--fix.For a locale like
es-419wheremergeLocaleistrue,loadJsonreturns the merged content ofes.json+es-419.json. AftersyncLocaleData, the result contains all base-language keys (fromes.json) that match the reference.writeFileSyncon line 213 then writes this entire merged result back toes-419.json, bloating it with keys that should only live ines.json.This matches the bug shuuji3 reported in the PR comments. The
forCountryflag is populated incheckCountryVariant(lines 76, 79) but never consumed anywhere in the processing or write path — it appears the guard to skip or differently handle country variant files during--fixis missing.Suggested approach
One option: skip writing back to country-variant files during
--fixinrunAllLocales, or filter them out entirely. For example, inprocessLocale:const newContent = syncLocaleData(referenceContent, targetContent, stats, fix) + // Don't rewrite country variant files — they should only contain overrides + if (localeInfo.mergeLocale) { + return stats + } + // Write if there are removals (always) or we are in fix mode if (stats.extra.length > 0 || fix) { writeFileSync(filePath, JSON.stringify(newContent, null, 2) + '\n', 'utf-8') }This would still report missing/extra keys for country variants without modifying their files. The exact behaviour depends on what you intend for
runSingleLocaletargeting a variant directly.
🧹 Nitpick comments (3)
scripts/compare-translations.ts (3)
148-154:delete result[key]is a no-op — the key was never assigned.When
Object.keys(data).length === 0,result[key]hasn't been set yet, so thedeletedoes nothing. Simplify by just not assigning:Suggested fix
const data = syncLocaleData(refValue, nextTarget, stats, fix, propertyPath) // don't add empty objects: --fix will prevent this - if (Object.keys(data).length === 0) { - delete result[key] - } else { + if (Object.keys(data).length > 0) { result[key] = data }
37-56:forCountryis populated but never read anywhere in the file.
populateLocaleCountriesandcheckCountryVariantsetforCountryonLocaleInfo, but no downstream code checks it. If it's intended to distinguish the "primary country" entry (e.g.es-ESwithcountry: true) from other variants, it needs to be consumed — likely in the write guard discussed above. Otherwise it's dead state that adds confusion.
340-347: Reference locale bypassescheckCountryVariant— intentional but worth a comment.The
enreference is loaded with a hand-craftedLocaleInfothat skipscheckJsonName/checkCountryVariant. This is correct (the reference shouldn't be validated the same way), but a brief inline comment would clarify intent for future readers.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/compare-translations.ts (1)
211-231:⚠️ Potential issue | 🔴 CriticalPrevent
--fixfrom writing merged base keys into country-variant files.When
--fixmode is used on locale variants likees-419.json, the script loads the merged content (combininges.jsonwith the variant's overrides) and then writes the full merged tree back to the variant file. This corrupts the file structure—es-419.jsonshould only contain translation overrides, not a complete copy of the base language content. Skip writes formergeLocale-backed variants by checkinglocaleInfo.mergeLocalebefore writing, or passfix && !localeInfo.mergeLocaleto prevent modifying variant files.
| await fs.writeFile( | ||
| path.resolve(`${destFolder}/${locale.code}.json`), | ||
| JSON.stringify(source, null, 2), | ||
| 'utf-8', |
There was a problem hiding this comment.
Preserve trailing newline to avoid dirty lunaria/files.
Line 90 writes JSON without a newline, which strips EOF newlines and can leave tracked files dirty after running the script, triggering build failures that require clean working trees. Please append \n when writing.
🛠️ Suggested fix
- JSON.stringify(source, null, 2),
+ JSON.stringify(source, null, 2) + '\n',
'utf-8',| const checkCountryVariant = (localeInfo: LocaleInfo): void => { | ||
| const { locale, lang, country } = localeInfo | ||
| const countryVariant = countries.get(lang) | ||
| if (countryVariant) { | ||
| if (country) { | ||
| const found = countryVariant.get(locale) | ||
| if (!found) { | ||
| console.error( | ||
| `${COLORS.red}Error: Invalid locale file "${locale}", it should be included at "countryLocaleVariants" in config/i18n.ts"${COLORS.reset}`, | ||
| ) | ||
| process.exit(1) | ||
| } | ||
| localeInfo.forCountry = found.forCountry | ||
| localeInfo.mergeLocale = found.mergeLocale | ||
| } else { | ||
| localeInfo.forCountry = false | ||
| localeInfo.mergeLocale = false | ||
| } | ||
| } else { | ||
| if (!country) { | ||
| console.error( | ||
| `${COLORS.red}Error: Invalid locale file "${locale}", it should be included at "countryLocaleVariants" in config/i18n.ts, or change the name to include country name "${lang}-<country-name>"${COLORS.reset}`, | ||
| ) | ||
| process.exit(1) |
There was a problem hiding this comment.
Remove the stray quote in the error message.
There’s an extra " before ${COLORS.reset} which leaks into the CLI output.
🛠️ Suggested fix
- `${COLORS.red}Error: Invalid locale file "${locale}", it should be included at "countryLocaleVariants" in config/i18n.ts"${COLORS.reset}`,
+ `${COLORS.red}Error: Invalid locale file "${locale}", it should be included at "countryLocaleVariants" in config/i18n.ts${COLORS.reset}`,
Co-authored-by: Daniel Roe <daniel@roe.dev>
3 participants
This PR includes:
mergeLocaleObjectutility fromlunaria/prepare-json-files.tsand use itconfig/i18n.tsconfig/i18n.tsContext: check autofix in this PR #1036
/cc @shuuji3