chore(clerk-js, localizations): Improve type safety of localizationKeys() with params
#6108
Conversation
🦋 Changeset detectedLatest commit: ea97fa8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis set of changes enhances type safety and clarity for localization key usage across multiple UI components and the localization infrastructure. The updates introduce stricter typing for interpolation parameters in localization strings, ensuring that required values are always provided and defaulting to empty strings when necessary to avoid undefined values. Several component props are made required instead of optional, and the localization function signatures are updated to leverage improved type inference for parameters. Additionally, the internal localization resource type is expanded with metadata to indicate required interpolation keys. Minor configuration adjustments, such as bundle size limits, are also included. Suggested labels
Suggested reviewers
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
panteliselef
requested review from
aeliox,
wobsoriano,
chanioxaris and
a team
as code owners
![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
🔭 Outside diff range comments (3)
packages/clerk-js/src/ui/components/OrganizationProfile/VerifyDomainForm.tsx (1)
88-90:⚠️ Potential issue
dataChangedcomparison is probably wrong
organization.name(the org display name) is compared withemailField.value(local-part of an email). These strings are unrelated, sodataChangedwill almost always betrue, enabling the submit button even when the input is untouched.-const dataChanged = organization.name !== emailField.value; +// Detect actual user edit +const dataChanged = emailField.isDirty(); // or compare against initial value snapshotConsider using the control’s own
isDirtyflag or storing the initial value for a meaningful equality check.packages/clerk-js/src/ui/components/OrganizationProfile/VerifiedDomainForm.tsx (1)
42-47:⚠️ Potential issueBug: wrong counter passed to “suggestion” label
totalInvitationsis used for both invitation and suggestion counts, so suggestion text will show an incorrect number.- localizationKeys(`organizationProfile.verifiedDomainPage.enrollmentTab.calloutSuggestionCountLabel`, { - count: totalInvitations, + localizationKeys(`organizationProfile.verifiedDomainPage.enrollmentTab.calloutSuggestionCountLabel`, { + count: totalSuggestions, }),packages/clerk-js/src/ui/components/OrganizationProfile/ActionConfirmationPage.tsx (1)
100-110:⚠️ Potential issue
organizationNameshould be required onceactionDescriptionrelies on it
canSubmitcompares user input withorganizationName; if it’sundefined, the form is permanently disabled.
Either:-organizationName?: string; +organizationName: string;or guard the comparison:
-const canSubmit = actionDescription ? confirmationField.value === organizationName : true; +const canSubmit = !organizationName || confirmationField.value === organizationName;
🧹 Nitpick comments (11)
.changeset/modern-bats-relate.md (1)
1-6: Replace the “wip” placeholder before mergeThe changeset is currently marked only with “wip”. If merged as-is, the release notes will be useless and automated versioning may skip or mis-label the bump.
Provide a concise human-readable summary (what changed, why) and, if relevant, note any breaking changes.Need help drafting the final changeset entry?
packages/clerk-js/src/ui/components/SignUp/SignUpStartAlternativePhoneCodePhoneNumberCard.tsx (1)
78-80: Redundant fallback –provideris already a string
phoneCodeProvider.nameis typed as a string, soprovider || ''will never hit the fallback.
If the intent is defensive coding, prefer the nullish coalescing operator to avoid masking''as a valid value:- provider: provider || '', + provider: provider ?? '',packages/clerk-js/src/ui/components/UserProfile/EmailForm.tsx (1)
88-90: Empty-string identifier can yield an awkward subtitle
identifier: ''makes the subtitle render as “… to ” if the ref has not been populated yet.
Consider delaying the second step until the e-mail is known or skip the parameter entirely:- headerSubtitle={localizationKeys(`${translationKey}.formSubtitle`, { - identifier: emailAddressRef.current?.emailAddress || '', - })} + headerSubtitle={ + emailAddressRef.current + ? localizationKeys(`${translationKey}.formSubtitle`, { + identifier: emailAddressRef.current.emailAddress, + }) + : undefined + }This keeps UX clean without compromising the new typings.
packages/clerk-js/src/ui/components/OrganizationList/OrganizationListPage.tsx (1)
181-183: Consider a more meaningful fallback than an empty application nameIf
applicationNameis undefined we now show “Select an organization for ”.
Either provide a i18n-friendly default (e.g. “this application”) or hide the interpolation:- applicationName: environment.displayConfig.applicationName || '', + ...(environment.displayConfig.applicationName + ? { applicationName: environment.displayConfig.applicationName } + : {})Slightly better UX with negligible code cost.
packages/clerk-js/src/ui/components/SignUp/SignUpPhoneCodeCard.tsx (1)
60-64: Deduplicate the provider fallbackYou compute
provideronce and then repeatprovider || ''twice.
Extract aproviderNameconstant to keep things dry and clearer for translators:- const provider = getAlternativePhoneCodeProviderData(channel)?.name; + const providerName = getAlternativePhoneCodeProviderData(channel)?.name ?? ''; - cardTitleKey = localizationKeys('signUp.alternativePhoneCodeProvider.title', { provider: provider || '' }); - cardSubtitleKey = localizationKeys('signUp.alternativePhoneCodeProvider.subtitle', { - provider: provider || '', - }); + cardTitleKey = localizationKeys('signUp.alternativePhoneCodeProvider.title', { provider: providerName }); + cardSubtitleKey = localizationKeys('signUp.alternativePhoneCodeProvider.subtitle', { provider: providerName });Minor, but increases readability.
packages/clerk-js/src/ui/components/OrganizationProfile/RemoveDomainForm.tsx (1)
65-66: Empty domain string might confuse usersRendering “… remove the domain ” looks odd. Prefer hiding the form until the domain is fetched, or omit the param when unavailable:
- domain: ref.current?.name || '', + ...(ref.current?.name ? { domain: ref.current.name } : {})Keeps the copy coherent and surfaces loading issues earlier.
packages/clerk-js/src/ui/components/SignIn/SignInFactorOneAlternativePhoneCodeCard.tsx (1)
16-19: Empty-string fallback may degrade UX
If no provider name is found, the title will render with an empty placeholder (e.g., “Enter the code sent by ”).
Consider displaying the channel itself or a generic fallback:-provider: getAlternativePhoneCodeProviderData(props.factor.channel)?.name || '', +provider: + getAlternativePhoneCodeProviderData(props.factor.channel)?.name ?? + props.factor.channel ?? + 'your provider',packages/clerk-js/src/ui/elements/LegalConsentCheckbox.tsx (1)
26-37: Remove stray comment & unify fallback logic
The inline “is this different than before ?” comment is noise. Also, consider using nullish-coalescing consistently.-// is this different than before ? -termsOfServiceLink: props.termsUrl || '', +termsOfServiceLink: props.termsUrl ?? '',Apply similarly to the other branches.
packages/clerk-js/src/ui/components/UserProfile/RemoveResourceForm.tsx (2)
101-108: Prefer nullish-coalescing and reuse a localproviderNamevariableUsing
|| ''masks legitimate falsy but defined values and duplicates the lookup.-identifier: providerToDisplayData[ref.current]?.name || '', +const providerName = providerToDisplayData[ref.current]?.name ?? ''; +... +identifier: providerName,Same applies to the
connectedAccountparameter a few lines below.
213-217: Same?? ''nit & potential TODO resolution
passkey.name ?? ''is clearer than|| ''. Ifpasskey.namecan’t be null, drop the fallback and the TODO altogether to avoid silently swallowing bad data.packages/types/src/localization.ts (1)
18-22: Edge case:nevergeneric still intersects withstring
string & { __params: ... }is assignable tostring, but the reverse is not, forcing casts in many call-sites.
Consider keeping a nominal wrapper instead of intersection to avoid widening pains and excessiveas unknown as.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.changeset/modern-bats-relate.md(1 hunks)packages/clerk-js/src/ui/components/OrganizationList/OrganizationListPage.tsx(1 hunks)packages/clerk-js/src/ui/components/OrganizationProfile/ActionConfirmationPage.tsx(4 hunks)packages/clerk-js/src/ui/components/OrganizationProfile/RemoveDomainForm.tsx(1 hunks)packages/clerk-js/src/ui/components/OrganizationProfile/VerifiedDomainForm.tsx(1 hunks)packages/clerk-js/src/ui/components/OrganizationProfile/VerifyDomainForm.tsx(1 hunks)packages/clerk-js/src/ui/components/PaymentSources/PaymentSources.tsx(1 hunks)packages/clerk-js/src/ui/components/PricingTable/PricingTableDefault.tsx(1 hunks)packages/clerk-js/src/ui/components/SignIn/SignInFactorOneAlternativePhoneCodeCard.tsx(1 hunks)packages/clerk-js/src/ui/components/SignUp/SignUpPhoneCodeCard.tsx(1 hunks)packages/clerk-js/src/ui/components/SignUp/SignUpStartAlternativePhoneCodePhoneNumberCard.tsx(1 hunks)packages/clerk-js/src/ui/components/UserProfile/EmailForm.tsx(1 hunks)packages/clerk-js/src/ui/components/UserProfile/MfaPhoneCodeScreen.tsx(1 hunks)packages/clerk-js/src/ui/components/UserProfile/PhoneForm.tsx(1 hunks)packages/clerk-js/src/ui/components/UserProfile/RemoveResourceForm.tsx(2 hunks)packages/clerk-js/src/ui/components/UserProfile/VerifyWithCode.tsx(1 hunks)packages/clerk-js/src/ui/elements/LegalConsentCheckbox.tsx(1 hunks)packages/clerk-js/src/ui/elements/SocialButtons.tsx(1 hunks)packages/clerk-js/src/ui/localization/localizationKeys.ts(2 hunks)packages/types/src/localization.ts(27 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (10)
packages/clerk-js/src/ui/components/SignUp/SignUpStartAlternativePhoneCodePhoneNumberCard.tsx (1)
packages/clerk-js/src/ui/localization/localizationKeys.ts (1)
localizationKeys(72-77)
packages/clerk-js/src/ui/components/PricingTable/PricingTableDefault.tsx (1)
packages/clerk-js/src/ui/localization/localizationKeys.ts (1)
localizationKeys(72-77)
packages/clerk-js/src/ui/components/SignUp/SignUpPhoneCodeCard.tsx (1)
packages/clerk-js/src/ui/localization/localizationKeys.ts (1)
localizationKeys(72-77)
packages/clerk-js/src/ui/components/OrganizationProfile/VerifiedDomainForm.tsx (1)
packages/clerk-js/src/ui/localization/localizationKeys.ts (1)
localizationKeys(72-77)
packages/clerk-js/src/ui/components/SignIn/SignInFactorOneAlternativePhoneCodeCard.tsx (1)
packages/shared/src/alternativePhoneCode.ts (1)
getAlternativePhoneCodeProviderData(10-15)
packages/clerk-js/src/ui/components/UserProfile/VerifyWithCode.tsx (1)
packages/clerk-js/src/ui/localization/localizationKeys.ts (1)
localizationKeys(72-77)
packages/clerk-js/src/ui/elements/SocialButtons.tsx (1)
packages/clerk-js/src/ui/localization/localizationKeys.ts (1)
localizationKeys(72-77)
packages/clerk-js/src/ui/components/UserProfile/RemoveResourceForm.tsx (1)
packages/clerk-js/src/ui/localization/localizationKeys.ts (1)
localizationKeys(72-77)
packages/clerk-js/src/ui/components/OrganizationProfile/ActionConfirmationPage.tsx (2)
packages/react/src/isomorphicClerk.ts (1)
organization(658-664)packages/clerk-js/src/ui/localization/localizationKeys.ts (1)
LocalizationKey(65-68)
packages/types/src/localization.ts (1)
packages/types/src/utils.ts (1)
DeepPartial(32-34)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
packages/clerk-js/src/ui/components/PaymentSources/PaymentSources.tsx (1)
85-91:❓ Verification inconclusive
Parameter name inconsistent – verify i18n template
messageLine1passes{ identifier: … }whilesuccessMessagenow passes{ paymentSource: … }.
If the underlying translation still expects{{identifier}}, users will see the raw placeholder.- successMessage={localizationKeys('userProfile.billingPage.paymentSourcesSection.removeResource.successMessage', { - paymentSource: ref.current, - })} + successMessage={localizationKeys( + 'userProfile.billingPage.paymentSourcesSection.removeResource.successMessage', + { identifier: ref.current }, // Keep param names aligned + )}Run the script below to check the expected variable names inside your locale files:
🏁 Script executed:
#!/bin/bash # Find occurrences of the successMessage key and print lines with interpolation tokens rg --no-heading -A2 -B2 'userProfile\.billingPage\.paymentSourcesSection\.removeResource\.successMessage'Length of output: 748
🏁 Script executed:
#!/bin/bash # Locate any JSON/YAML locale files defining the removeResource section rg -n '"removeResource"' --glob '*.json' --glob '*.yml' --glob '*.yaml' # Within those files, search for the successMessage entry and inspect its interpolation tokens rg -n '"successMessage"' --glob '*.json' --glob '*.yml' --glob '*.yaml'Length of output: 147
Parameter name mismatch – confirm i18n template
I couldn’t locate theremoveResource.successMessageentry in our JSON/YAML locale files, so it’s unclear whether the translation expects{{identifier}}or{{paymentSource}}. Please double-check your translation definitions and align the prop accordingly to avoid raw placeholders:• If the template uses
identifier, update the component to:- successMessage={localizationKeys( - 'userProfile.billingPage.paymentSourcesSection.removeResource.successMessage', - { paymentSource: ref.current }, - )} + successMessage={localizationKeys( + 'userProfile.billingPage.paymentSourcesSection.removeResource.successMessage', + { identifier: ref.current }, // match your translation key + )}• If the template uses
paymentSource, update your locale file to referencepaymentSourceinstead.
Please verify and adjust either the component or the translation file as needed.packages/clerk-js/src/ui/components/UserProfile/PhoneForm.tsx (1)
133-134: LGTM – avoids undefined in interpolationProviding
''as a fallback guarantees the string placeholder is always satisfied and keeps runtime safe.packages/clerk-js/src/ui/components/UserProfile/MfaPhoneCodeScreen.tsx (1)
153-155: LGTM – consistent with new localization typingSame rationale as above; the empty-string fallback aligns with the stricter
localizationKeystyping.packages/clerk-js/src/ui/components/PricingTable/PricingTableDefault.tsx (1)
254-257:❓ Verification inconclusive
Avoid rendering a “starts at” notice when the date is missing
Silently falling back to an empty string means the UI could render “Starts at ”, which looks broken and makes debugging harder.
Instead, guard the wholeTextnode (or passundefined) when the date is unavailable.- localizationKey={localizationKeys('badge__startsAt', { - // TODO: Is it correct to be undefined ? - date: subscription?.periodStart || '', - })} + localizationKey={ + subscription?.periodStart + ? localizationKeys('badge__startsAt', { date: subscription.periodStart }) + : undefined + }This preserves the improved type-safety while preventing an empty placeholder from leaking to users.
🏁 Script executed:
#!/bin/bash set -e FILE="packages/clerk-js/src/ui/components/PricingTable/PricingTableDefault.tsx" if [[ -f "$FILE" ]]; then sed -n '240,300p' "$FILE" else echo "File not found: $FILE" exit 1 fiLength of output: 2611
🏁 Script executed:
#!/bin/bash set -e # 1. Show import statements to locate Text source sed -n '1,40p' packages/clerk-js/src/ui/components/PricingTable/PricingTableDefault.tsx # 2. Find Text component implementation in ui/components rg -R "export (const|function) Text" -n packages/clerk-js/src/ui/components # 3. Search for its props definition to see if localizationKey is optional rg -R "interface TextProps" -n packages/clerk-js/src/ui/components || rg -R "type TextProps" -n packages/clerk-js/src/ui/componentsLength of output: 1768
🏁 Script executed:
#!/bin/bash set -e # Locate the Text component file under customizables TEXT_FILES=$(fd -t f 'Text.*\.tsx' packages/clerk-js/src/ui/customizables || true) if [[ -z "$TEXT_FILES" ]]; then echo "No Text component files found in customizables." exit 0 fi # Print the file paths and their top 200 lines to inspect props for f in $TEXT_FILES; do echo "===== $f =====" sed -n '1,200p' "$f" doneLength of output: 214
#!/bin/bash set -e FILE="packages/clerk-js/src/ui/components/PricingTable/PricingTableDefault.tsx" # Find the line number where shouldShowFooterNotice is defined or first used grep -n "shouldShowFooterNotice" "$FILE" # Print 20 lines before and after the first occurrence of shouldShowFooterNotice LINE=$(grep -n "shouldShowFooterNotice" "$FILE" | head -n1 | cut -d: -f1) START=$((LINE - 20)) END=$((LINE + 20)) sed -n "${START},${END}p" "$FILE"
Guard the “Starts at” badge when the date is missing
Silently falling back to
''can result in a “Starts at ” label with no date. Instead, only pass alocalizationKeywhensubscription.periodStartis defined:• File:
packages/clerk-js/src/ui/components/PricingTable/PricingTableDefault.tsx
Wrap thelocalizationKeyprop in a presence check:- localizationKey={localizationKeys('badge__startsAt', { - // TODO: Is it correct to be undefined ? - date: subscription?.periodStart || '', - })} + localizationKey={ + subscription?.periodStart + ? localizationKeys('badge__startsAt', { date: subscription.periodStart }) + : undefined + }• Verify that the
Textcomponent’slocalizationKeyprop is optional to safely acceptundefined.packages/clerk-js/src/ui/components/OrganizationProfile/VerifyDomainForm.tsx (1)
50-55: Good defensive default foremailAddressparam
Passing''avoidsundefinedleaking into the interpolation map and satisfies the stricter typings introduced inlocalizationKeys().packages/clerk-js/src/ui/components/OrganizationProfile/VerifiedDomainForm.tsx (1)
131-136: Consistent string fallback looks good
Usingdomain?.name || ''aligns with the new non-nullable parameter contract.packages/clerk-js/src/ui/components/OrganizationProfile/ActionConfirmationPage.tsx (1)
129-137: Label mismatch risk
label: actionDescriptionpasses a full localization object where a simple string is usually displayed. Confirm the form control can render rich localization keys; otherwise the field label may break.
packages/clerk-js/src/ui/components/UserProfile/VerifyWithCode.tsx
Outdated
Show resolved
Hide resolved
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
packages/clerk-js/src/ui/components/OrganizationProfile/ActionConfirmationPage.tsx(4 hunks)packages/clerk-js/src/ui/components/PricingTable/PricingTableDefault.tsx(1 hunks)packages/clerk-js/src/ui/components/UserProfile/RemoveResourceForm.tsx(2 hunks)packages/clerk-js/src/ui/components/UserProfile/VerifyWithCode.tsx(1 hunks)packages/clerk-js/src/ui/elements/LegalConsentCheckbox.tsx(1 hunks)packages/types/src/localization.ts(27 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/clerk-js/src/ui/elements/LegalConsentCheckbox.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/clerk-js/src/ui/components/UserProfile/VerifyWithCode.tsx
- packages/clerk-js/src/ui/components/UserProfile/RemoveResourceForm.tsx
- packages/clerk-js/src/ui/components/PricingTable/PricingTableDefault.tsx
- packages/clerk-js/src/ui/components/OrganizationProfile/ActionConfirmationPage.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/types/src/localization.ts (1)
packages/types/src/utils.ts (1)
DeepPartial(32-34)
🔇 Additional comments (2)
packages/types/src/localization.ts (2)
18-49: Remove large blocks of commented-out experimental codeLines 18-49 are previous experiments of
LocalizationValue. Keeping them in the source adds noise and hurts maintainability; Git already preserves history.[ suggest_nitpick ]
14-16: Consider includingnull | undefinedinUnionToRecordWithPrimitivesInterpolated parameters often originate from optional fields (
birthday?: Date).
Extending the primitive union avoids extra casts at call-sites:- [K in T]: string | number | boolean | Date; + [K in T]: string | number | boolean | Date | null | undefined;[ suggest_optional_refactor ]
| export type LocalizationValue<T extends string = never, Constraint extends string = string> = [T] extends [never] | ||
| ? Constraint | ||
| : Constraint & { __params: UnionToRecordWithPrimitives<T> }; | ||
|
|
There was a problem hiding this comment.
LocalizationValue makes plain strings invalid – mark __params optional
Intersecting string with an object that contains a required __params field means a raw string can never satisfy the type:
type Foo = LocalizationValue<'provider'>;
declare const x: Foo;
// ❌ Type 'string' is not assignable to type '{ __params: … }'As a consequence every consumer-provided translation now needs to fabricate an artificial __params property.
Making the field optional keeps the compile-time inference for callers of localizationKeys() while still allowing simple string literals in resource files.
- : Constraint & { __params: UnionToRecordWithPrimitives<T> };
+ : Constraint & { __params?: UnionToRecordWithPrimitives<T> };🤖 Prompt for AI Agents
In packages/types/src/localization.ts around lines 31 to 34, the
LocalizationValue type currently requires the __params field, which makes plain
strings invalid as they do not have this property. To fix this, make the
__params field optional by marking it with a question mark. This allows raw
string literals to satisfy the type while preserving type inference for callers
of localizationKeys().
| type DeepLocalizationWithoutObjects<T> = { | ||
| [K in keyof T]: T[K] extends LocalizationValue<any> | ||
| ? T[K] | ||
| : T[K] extends object | ||
| ? DeepLocalizationWithoutObjects<T[K]> | ||
| : T[K]; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DeepLocalizationWithoutObjects doesn't do what the doc-comment promises
The comment states it “replac(es) all LocalizationValue types with their string representation”, yet the implementation leaves the original type untouched:
T[K] extends LocalizationValue<any> ? T[K] : …This breaks the public contract of LocalizationResource; resource authors will still be forced to return the branded type instead of a plain string.
- [K in keyof T]: T[K] extends LocalizationValue<any>
- ? T[K]
+ [K in keyof T]: T[K] extends LocalizationValue<any>
+ ? stringYou may also want to add readonly support (arrays, tuples) in the recursion while you are here.
🤖 Prompt for AI Agents
In packages/types/src/localization.ts around lines 76 to 82, the
DeepLocalizationWithoutObjects type does not convert LocalizationValue types to
strings as the comment states; it currently leaves them unchanged. Update the
conditional type to replace T[K] extends LocalizationValue<any> with string
instead of T[K]. Additionally, enhance the recursion to handle readonly arrays
and tuples by adding support for readonly modifiers in the type mapping to
ensure full coverage of nested structures.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/types/src/localization.ts (2)
18-21:⚠️ Potential issue
__paramsmust be optional to keep plain strings assignableRe-introducing the same issue highlighted in the previous review: intersecting
stringwith an object that has a required__paramsproperty prevents plain strings from satisfyingLocalizationValue.
Make the field optional so resource authors can still return raw strings.- : Constraint & { __params: UnionToRecordWithPrimitives<T> }; + : Constraint & { __params?: UnionToRecordWithPrimitives<T> };
47-52: 🛠️ Refactor suggestion
DeepLocalizationWithoutObjectsstill returns branded types instead ofstringThe doc-comment promises that every
LocalizationValueis replaced by its string representation, yet the implementation leaves the original type intact.
Update the conditional branch accordingly; while at it you can tighten the recursion to supportreadonlyarrays/tuples.- [K in keyof T]: T[K] extends LocalizationValue<any> - ? T[K] + [K in keyof T]: T[K] extends LocalizationValue<any> + ? string(Optionally recurse through
readonly (infer U)[]to preserve immutability.)
🧹 Nitpick comments (1)
packages/types/src/localization.ts (1)
47-52: Array & tuple handling missing in deep transform
T[K] extends objectwill also match arrays/functions, producing unexpected results or circularity.
Consider a dedicated branch forreadonly any[]/any[]to map element types instead of properties.Example sketch (pseudo-diff):
type DeepLocalizationWithoutObjects<T> = T extends readonly (infer U)[] ? readonly DeepLocalizationWithoutObjects<U>[] : T extends (infer U)[] ? DeepLocalizationWithoutObjects<U>[] : { [K in keyof T]: ... };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/types/src/localization.ts(27 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/types/src/localization.ts (1)
packages/types/src/utils.ts (1)
DeepPartial(32-34)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
# Conflicts: # packages/clerk-js/bundlewatch.config.json # packages/clerk-js/src/ui/components/PaymentSources/PaymentSources.tsx # pnpm-lock.yaml
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.changeset/cold-turtles-press.md (2)
1-3: Validate changeset header format
The front-matter correctly marks@clerk/typesas a minor bump and uses valid YAML quoting. For consistency, verify that all existing changesets use the same quoting style (single vs. double quotes) for package names.
5-5: Enhance description context
The summary is accurate; consider adding a brief note on where consumers will see benefits (e.g., improved IDE auto-completion or stricter compile-time errors) to make the change purpose more discoverable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/cold-turtles-press.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
I already discussed this with Alex offline, but why not share an answer with everyone. This PR does not affect the publicly exposed type |
Description
This PR improves the type safety of the
localizationKeys()utility, by exposing a new internal only type that the@clerk/clerk-jspackage consumes. The__internal_LocalizationResourcetype can include metadata for each localizatoin key and how to apply interpolation.Example
if a keys is types as
LocalizationValue<'firstName' | 'lastName'>, now any consumer oflocalizationKeysis required to doThis PR does not affect the publicly exposed type
LocalizationResourcefrom@clerk/types. Hence it is not a breaking change.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit