fix(plugin-i18n): change to builtinResources option - #296
Conversation
WalkthroughRenames the i18n plugin option from resources to builtinResources across implementation, types, tests, and documentation. No control-flow or behavioral changes; only the public option name is updated. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
@gunshi/bone
@gunshi/definition
gunshi
@gunshi/plugin
@gunshi/plugin-completion
@gunshi/plugin-dryrun
@gunshi/plugin-global
@gunshi/plugin-i18n
@gunshi/plugin-renderer
@gunshi/resources
@gunshi/shared
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/plugin-i18n/README.md (2)
82-89: Document the deprecation/migration path for the renameThe rename to
builtinResourcesis clear. Please add a short migration note indicating that the formerresourcesoption is deprecated and how to migrate. This reduces friction for users upgrading.Example addition:
> [!IMPORTANT] > Migration: The `resources` option has been renamed to `builtinResources`. `resources` will continue to work for one release as a deprecated alias. Please update your configuration accordingly.
123-126: Minor phrasing improvement in the inline commentTweak the comment for clarity.
Apply this diff in the snippet:
- builtinResources: { - 'ja-JP': jsJPResource // Set from with providing gunshi built-in resources - } + builtinResources: { + 'ja-JP': jsJPResource // Set using Gunshi built-in resources + }packages/plugin-i18n/src/index.test.ts (1)
72-72: LGTM: test updated to passbuiltinResourcesThe test now targets the new option and validates ja-JP built-ins correctly.
If you add a deprecated alias for
resources, consider a backward-compatibility test like:test('custom locale via deprecated resources alias still works', async () => { const jaJP = await import('@gunshi/resources/ja-JP', { with: { type: 'json' } }).then(m => m.default || m) // @ts-expect-error deprecated alias const plugin = i18n({ locale: 'ja-JP', resources: { 'ja-JP': jaJP } }) const ctx = await createMockCommandContext() const ext = await plugin.extension.factory(ctx, {} as Command) expect(ext.translate(resolveBuiltInKey('USAGE'))).toBe('使い方') })Happy to open a follow-up PR to add the BC test once the alias is implemented.
packages/plugin-i18n/src/index.ts (1)
153-155: LGTM: iterating provided built-ins is correct; consider whether overriding defaults should be allowedThe loop is correct. Note that
setResourcecurrently no-ops when a locale exists, so providingen-USinbuiltinResourcescannot overrideDefaultResource. If intentional, great; if not, we can allow explicit override.If you want to allow overrides for user-provided locales, update
setResourceto accept a thirdoverride = falseparameter and passtruehere. Example (outside this hunk):// change signature function setResource(locale: string | Intl.Locale, resource: Record<BuiltinResourceKeys, string>, override = false): void { const targetLocale = toLocale(locale) const targetLocaleStr = toLocaleString(targetLocale) if (!override && localeBuiltinResources.has(targetLocaleStr)) return localeBuiltinResources.set(targetLocaleStr, mapResourceWithBuiltinKey(resource)) } // call site (this hunk) for (const [locale, resource] of Object.entries(builtinResources)) { setResource(locale, resource, true) }Before changing behavior, verify whether overriding the default built-ins is desired by your users. If not, keep current behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/gunshi/src/cli.test.ts(1 hunks)packages/plugin-i18n/README.md(2 hunks)packages/plugin-i18n/src/index.test.ts(1 hunks)packages/plugin-i18n/src/index.ts(2 hunks)packages/plugin-i18n/src/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.ts: Use ES modules throughout the codebase
Follow existing code style (enforced by ESLint and Prettier)
Files:
packages/plugin-i18n/src/types.tspackages/gunshi/src/cli.test.tspackages/plugin-i18n/src/index.tspackages/plugin-i18n/src/index.test.ts
packages/gunshi/src/**/*.ts
📄 CodeRabbit Inference Engine (CLAUDE.md)
packages/gunshi/src/**/*.ts: All source code is in TypeScript with strict mode enabled
Type safety is a core feature - maintain strict TypeScript types throughout
Files:
packages/gunshi/src/cli.test.ts
🧠 Learnings (1)
📚 Learning: 2025-07-21T07:12:47.997Z
Learnt from: CR
PR: kazupon/gunshi#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T07:12:47.997Z
Learning: Applies to packages/gunshi/test/**/*.test.ts : Test files are in 'packages/gunshi/test/' with '.test.ts' extension
Applied to files:
packages/gunshi/src/cli.test.ts
🧬 Code Graph Analysis (3)
packages/plugin-i18n/src/types.ts (1)
packages/shared/src/types.ts (1)
BuiltinResourceKeys(56-56)
packages/plugin-i18n/src/index.ts (1)
playground/i18n/index.js (2)
locale(42-42)resource(31-31)
packages/plugin-i18n/src/index.test.ts (1)
packages/plugin-i18n/src/index.ts (1)
i18n(79-212)
| builtinResources: { 'ja-JP': jsJPResource } | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM: test config aligns with the new public option
Switching to builtinResources matches the plugin API rename and keeps tests up to date.
Run this repo-wide check to ensure there are no lingering usages of the old resources option in code, tests, or docs:
🏁 Script executed:
#!/bin/bash
# Find any remaining references to the old option name
rg -n --hidden --glob '!.git' --glob '!node_modules' --glob '!dist' $'\\bresources\\s*:' || true
# Confirm new option is used
rg -n --hidden --glob '!.git' --glob '!node_modules' --glob '!dist' $'\\bbuiltinResources\\s*:'Length of output: 685
🏁 Script executed:
#!/bin/bash
# Inspect plugin-renderer source for "resources" usage
echo "=== plugin-renderer/src ==="
rg -n "resources" packages/plugin-renderer/src
# Show context in plugin-renderer README around the snippet
echo -e "\n=== plugin-renderer/README.md (lines 230-260) ==="
sed -n '230,260p' packages/plugin-renderer/README.md
# Show translation.ts around the private field
echo -e "\n=== plugin-i18n/src/translation.ts (lines 1-40) ==="
sed -n '1,40p' packages/plugin-i18n/src/translation.tsLength of output: 2426
Documentation update required: replace deprecated resources option in plugin-renderer README
The test changes correctly switch to the new builtinResources API, but the docs still reference the old resources option. Please update:
• packages/plugin-renderer/README.md (around line 243):
- resources: customResources
+ builtinResources: customResourcesNo changes are needed for the private #resources field in plugin-i18n/src/translation.ts or the lockfile entry.
📝 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.
| builtinResources: { 'ja-JP': jsJPResource } | |
| }) | |
| --- packages/plugin-renderer/README.md | |
| @@ -243,1 +243,1 | |
| - resources: customResources | |
| + builtinResources: customResources |
🤖 Prompt for AI Agents
In packages/plugin-renderer/README.md around line 243, the documentation still
references the deprecated resources option; update all examples and option docs
to use builtinResources instead (e.g., replace "resources: { 'ja-JP': ... }"
with "builtinResources: { 'ja-JP': ... }"), search the README for any other
occurrences of "resources" related to this plugin-renderer API and replace them,
and ensure the example code blocks and descriptive text reflect the new API name
and usage consistently.
| const builtinResources = | ||
| options.builtinResources || | ||
| (Object.create(null) as Record<string, Record<BuiltinResourceKeys, string>>) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide a backward-compatible fallback with a deprecation warning
To avoid breaking existing users immediately, fall back to the deprecated resources option when builtinResources is not provided, and warn once. This pairs with the type alias change.
Apply this diff:
- const builtinResources =
- options.builtinResources ||
- (Object.create(null) as Record<string, Record<BuiltinResourceKeys, string>>)
+ const builtinResources =
+ options.builtinResources ??
+ // Deprecated alias (to be removed in the next major)
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ (options as any).resources ??
+ (Object.create(null) as Record<string, Record<BuiltinResourceKeys, string>>)
+
+ // Soft deprecation notice (printed once)
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ if ((options as any).resources && !options.builtinResources) {
+ // Keep this lightweight to avoid noisy logs; consider gating by env if needed
+ console.warn(
+ '[@gunshi/plugin-i18n] `resources` option is deprecated. Use `builtinResources` instead.'
+ )
+ }📝 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.
| const builtinResources = | |
| options.builtinResources || | |
| (Object.create(null) as Record<string, Record<BuiltinResourceKeys, string>>) | |
| const builtinResources = | |
| options.builtinResources ?? | |
| // Deprecated alias (to be removed in the next major) | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| (options as any).resources ?? | |
| (Object.create(null) as Record<string, Record<BuiltinResourceKeys, string>>) | |
| // Soft deprecation notice (printed once) | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| if ((options as any).resources && !options.builtinResources) { | |
| // Keep this lightweight to avoid noisy logs; consider gating by env if needed | |
| console.warn( | |
| '[@gunshi/plugin-i18n] `resources` option is deprecated. Use `builtinResources` instead.' | |
| ) | |
| } |
🤖 Prompt for AI Agents
In packages/plugin-i18n/src/index.ts around lines 86 to 88, the code currently
defaults builtinResources to an empty object which breaks backward
compatibility; update it to fall back to the deprecated resources option when
builtinResources is undefined, and emit a single deprecation warning the first
time this fallback is used. Implement a one-time warning (e.g., using a
module-scoped boolean flag) so the log only appears once, set builtinResources =
options.builtinResources ?? options.resources ?? Object.create(null), and call
the warning when options.builtinResources is undefined but options.resources is
provided.
| builtinResources?: Record<string, Record<BuiltinResourceKeys, string>> | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Public option rename looks good; add a deprecated alias to avoid a breaking change
Renaming a public option is a breaking change for downstream consumers. Consider keeping a deprecated resources? alias in the type to provide a migration window while promoting builtinResources. This preserves strict typing and eases adoption.
Apply this diff to introduce a deprecated alias:
export interface I18nPluginOptions {
/**
* Locale to use for translations
*/
locale?: string | Intl.Locale
/**
* Translation adapter factory
*/
translationAdapterFactory?: TranslationAdapterFactory
/**
* Built-in localizable resources
*/
- builtinResources?: Record<string, Record<BuiltinResourceKeys, string>>
+ /**
+ * @deprecated Use `builtinResources` instead. This alias will be removed in the next major release.
+ */
+ resources?: Record<string, Record<BuiltinResourceKeys, string>>
+ builtinResources?: Record<string, Record<BuiltinResourceKeys, 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.
| builtinResources?: Record<string, Record<BuiltinResourceKeys, string>> | |
| } | |
| export interface I18nPluginOptions { | |
| /** | |
| * Locale to use for translations | |
| */ | |
| locale?: string | Intl.Locale | |
| /** | |
| * Translation adapter factory | |
| */ | |
| translationAdapterFactory?: TranslationAdapterFactory | |
| /** | |
| * Built-in localizable resources | |
| */ | |
| /** | |
| * @deprecated Use `builtinResources` instead. This alias will be removed in the next major release. | |
| */ | |
| resources?: Record<string, Record<BuiltinResourceKeys, string>> | |
| builtinResources?: Record<string, Record<BuiltinResourceKeys, string>> | |
| } |
🤖 Prompt for AI Agents
In packages/plugin-i18n/src/types.ts around lines 93-94, the public option was
renamed to builtinResources which is a breaking change for consumers; add an
optional deprecated alias resources with the identical type signature
(Record<string, Record<BuiltinResourceKeys, string>>) so existing callers keep
working; annotate the alias with a JSDoc @deprecated comment pointing users to
use builtinResources and keep both properties optional to preserve strict typing
and provide a migration window.
1 participant
Description
Make the strictly option names for
pluign-i18nfiner to reduce cognitive load.Linked Issues
Additional context
Summary by CodeRabbit