fix(i18n): optimize i18n types performance #870
Conversation
✅ Deploy Preview for livecodes ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughDeleted Node.js type-check CI workflow and tsconfig.full.json, removed root Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (5)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tsconfig.json (1)
51-53: Locales now type-checked; consider skipLibCheck and align checkJs with include.Including
src/livecodes/i18n/locales/**is likely intended for the new i18n typing. For faster builds, considerskipLibCheck. Also,allowJs: true+checkJs: truewon’t have effect withinclude: ["src/**/*.ts"].Apply:
"compilerOptions": { + "skipLibCheck": true, "rootDir": "src",If you want JS in type-checks, broaden include:
- "include": ["src/**/*.ts"], + "include": ["src"],Or disable JS checks to avoid confusion:
- "checkJs": true, + "checkJs": false,Please confirm intended scope (TS-only vs TS+JS) and measure
tsc --noEmit --extendedDiagnosticsbefore/after.package.json (1)
71-72: Type-check CI guard removed; add a lightweight script to keep an easy gate.With
check-typesand the CI workflow removed, ensure another job runstsc. Add a generic script so workflows can call it.Apply:
"scripts": { @@ - "i18n-lokalise-json": "node ./scripts/i18n-lokalise-json.mjs" + "i18n-lokalise-json": "node ./scripts/i18n-lokalise-json.mjs", + "typecheck": "tsc -p tsconfig.json --noEmit" },Then wire
npm run typecheckinto an existing CI workflow (build/test job).src/livecodes/i18n/locales/models.ts (1)
5-16: Doc path nit: scripts directory name.Comment references
script/vscode-intellisense.jsbut repo scripts live underscripts/.Apply:
- * To add new custom data attributes for HTML intellisense, see `script/vscode-intellisense.js`. + * To add new custom data attributes for HTML intellisense, see `scripts/vscode-intellisense.js`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/CI-type-checking.yml(0 hunks)package.json(1 hunks)src/livecodes/i18n/locales/models.ts(2 hunks)src/livecodes/i18n/models.ts(1 hunks)tsconfig.full.json(0 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/CI-type-checking.yml
- tsconfig.full.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (18.x)
- GitHub Check: tests (18.x, 4)
- GitHub Check: tests (18.x, 5)
- GitHub Check: tests (18.x, 3)
- GitHub Check: tests (18.x, 1)
- GitHub Check: tests (18.x, 2)
🔇 Additional comments (4)
src/livecodes/i18n/models.ts (1)
6-12: RequireAtLeastOne looks good and distributive; verify empty-object behavior is desired.This returns
neverforkeyof T extends never. IfTcould be{}somewhere, that’ll surface as an error. Confirm that’s intended.src/livecodes/i18n/locales/models.ts (3)
24-25: Template now allows nested objects — good for structured keys.This aligns with
as const satisfiesusage foren. No issues.
44-45: Verify LangInfoTranslation isn't an array or use an array-safe I18nStructurerg returned no hits for array assignments in src/livecodes/i18n/locales/en/language-info.ts — I couldn't confirm. Check src/livecodes/i18n/locales/models.ts (lines 44–45): if LangInfoTranslation contains arrays, convert them to object maps or switch to the array-safe I18nStructure.
39-40: No arrays found in en translations — typing is safe. Inspected src/livecodes/i18n/locales/en/translation.ts and translation.lokalise.json; no array literals in leaf values were found, so I18nStructure models the translations correctly.
| /** | ||
| * Maps a nested object structure to a structure where all leaf nodes are strings. | ||
| * | ||
| * Use to keep the same structure as the `en` i18n object for other languages. | ||
| */ | ||
| export type I18nStructure<T> = { | ||
| readonly [K in keyof T]: T[K] extends Record<string, any> ? I18nStructure<T[K]> : string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle arrays in I18nStructure to avoid treating them as objects.
Record<string, any> matches arrays, which would map array methods/indices into the structure. Prefer explicit array handling and avoid any.
Apply:
-export type I18nStructure<T> = {
- readonly [K in keyof T]: T[K] extends Record<string, any> ? I18nStructure<T[K]> : string;
-};
+export type I18nStructure<T> = {
+ readonly [K in keyof T]:
+ // Preserve arrays as arrays of strings
+ T[K] extends readonly (infer _E)[]
+ ? readonly string[]
+ // Recurse into plain objects
+ : T[K] extends Record<string, unknown>
+ ? I18nStructure<T[K]>
+ : string;
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Maps a nested object structure to a structure where all leaf nodes are strings. | |
| * | |
| * Use to keep the same structure as the `en` i18n object for other languages. | |
| */ | |
| export type I18nStructure<T> = { | |
| readonly [K in keyof T]: T[K] extends Record<string, any> ? I18nStructure<T[K]> : string; | |
| }; | |
| /** | |
| * Maps a nested object structure to a structure where all leaf nodes are strings. | |
| * | |
| * Use to keep the same structure as the `en` i18n object for other languages. | |
| */ | |
| export type I18nStructure<T> = { | |
| readonly [K in keyof T]: | |
| // Preserve arrays as arrays of strings | |
| T[K] extends readonly (infer _E)[] | |
| ? readonly string[] | |
| // Recurse into plain objects | |
| : T[K] extends Record<string, unknown> | |
| ? I18nStructure<T[K]> | |
| : string; | |
| }; |
🤖 Prompt for AI Agents
In src/livecodes/i18n/locales/models.ts around lines 27 to 34, the I18nStructure
type currently treats arrays as Record<string, any>, which causes array indices
and methods to be mapped and uses `any`. Change the conditional type to
explicitly detect arrays (T[K] extends Array<infer U> ? I18nStructure<U>[] ...)
before the object check, and replace `any` with `unknown` in the object branch
(T[K] extends Record<string, unknown> ? I18nStructure<T[K]> : string) so arrays
map to arrays of leaf strings/structures and objects use a safe unknown type
instead of any.
|
|
That's very nice. |
i18n ActionsSource PR has been merged into the default branch. Maintainers can comment |
|
.i18n-update-push |
i18n Actions:
|
| Name | Description |
|---|---|
| New Branch for i18n | i18n/zyf722/i18next-integration-draft |
| Last Commit SHA | c8acd7a |
Maintainers can comment .i18n-update-pull after translation is done to trigger the i18n pull workflow and pull the changes back to Github.
|
.i18n-update-pull |
What type of PR is this? (check all applicable)
Description
#707 (comment)
This PR uses a new solution to check type-safety for i18n locale ts files (which is used to check languages other than
ento keep consistent withen). The main change was to replace theUnAsConstutility type and uses a more direct indexed mapped type to recursively validate the structure.Related Tickets & Documents
Added tests?
Added to documentations?
Summary by CodeRabbit