chore: update compare-translations.ts logic#1063
chore: update compare-translations.ts logic#1063userquin wants to merge 7 commits intonpmx-dev:mainfrom
compare-translations.ts logic#1063Conversation
|
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! |
📝 WalkthroughWalkthroughAdds an exported 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 |
![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 }
1 participant
This PR includes:
mergeLocaleObjectutility fromlunaria/prepare-json-files.tsand use itconfig/i18n.tsconfig/i18n.tsContext: check autofix in this PR #1036
/cc @shuuji3