-
Notifications
You must be signed in to change notification settings - Fork 183
refactor:converted i18n.js to TypeScript #681
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe i18n module is converted from JavaScript to TypeScript, maintaining identical functionality while adding type safety. The module initializes banana-i18n, configures locale handling from the global window object, loads English messages as default with dynamic locale-specific message loading via require, and exports the configured Banana instance. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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: 0
🧹 Nitpick comments (3)
src/simulator/src/i18n.ts (3)
1-4: Use lightweight type declarations instead of@ts-ignoreon importsThese
@ts-ignoredirectives effectively makeBananaandenMessagesany, which undercuts the goal of adding type safety. In a follow-up, consider adding a small ambient declaration forbanana-i18nand JSON modules (e.g.,declare module 'banana-i18n' { ... }anddeclare module '*.json' { ... }or enablingresolveJsonModule) so you can drop these ignores or at least convert them to@ts-expect-errorwith a clear TODO.
6-24: GlobalWindowaugmentation is correct;Messagestyping could be tighter if shape is knownUsing
declare global { interface Window { locale?: string } }inside this module is the right pattern to satisfywindow.localeusage. TheMessagesindex signature asRecord<string, unknown>is safe but quite loose; if your locale JSON files are plain key→string maps (or some known structure), tightening this to a more specific type (e.g.,Record<string, string>or a dedicatedMessageMaptype) would give better autocomplete and earlier errors without changing runtime behavior.
25-32: Silent catch on dynamic localerequiremay hide configuration problemsThis
try/catchkeeps behavior the same—falling back to the preloadedenmessages when a locale JSON can’t be required—but swallowingerrentirely can make missing/broken locale bundles hard to diagnose. In a future cleanup, you might at least log aconsole.warnwith the failinglocale, or only attempt therequirewhenlocale !== finalFallback, to make misconfigurations easier to spot while preserving the fallback semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/i18n.js(0 hunks)src/simulator/src/i18n.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/simulator/src/i18n.js
🔇 Additional comments (1)
src/simulator/src/i18n.ts (1)
34-35: Initialization and export preserve original behavior; just verify imports and runtimeThe sequence
setLocale→ derivelocale→ buildmessages→load→ default export matches the described JS behavior, so consumers should see the same preconfigured Banana instance. It’s worth double-checking that no code still importsi18n.jsexplicitly and that locale switching still works at runtime.You can quickly verify call sites and extensions with:
#!/bin/bash # Ensure no stale imports still reference i18n.js rg -n "i18n\\.js" . # Spot-check references to this module (should generally be extensionless: './i18n') rg -n "['\"]/i18n['\"]" src

Fixes #661
Describe the changes you have made in this PR -
Converted
i18n.jsto TypeScript while maintaining the logic and functionality sameNote: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.