fix(typing): n() & d() output depending "part" option #2193
Conversation
WalkthroughThe changes update type definitions for date and number formatting methods in the core and Vue integration packages. They introduce a conditional type alias to dynamically determine the return type based on the presence and value of a Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/vue-i18n/src/vue.d.ts (1)
607-620: Minor type parameter inconsistency.The
OptionsTypegeneric is defined with a default value but the actualoptionsparameter on line 618 still usesDateTimeOptions<Key | ResourceKeys>directly instead ofOptionsType. Consider updating line 618 to useOptionsTypefor consistency:- options: DateTimeOptions<Key | ResourceKeys>, + options: OptionsType,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/vue-i18n-core/src/composer.ts(5 hunks)packages/vue-i18n-core/test/composer.test-d.ts(2 hunks)packages/vue-i18n/src/vue.d.ts(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/vue-i18n/src/vue.d.ts (2)
packages/core-base/src/datetime.ts (1)
DateTimeOptions(80-103)packages/core-base/src/number.ts (1)
NumberOptions(78-101)
packages/vue-i18n-core/src/composer.ts (3)
packages/vue-i18n-core/src/index.ts (2)
DateTimeOptions(7-7)NumberOptions(30-30)packages/core-base/src/datetime.ts (1)
DateTimeOptions(80-103)packages/core-base/src/number.ts (1)
NumberOptions(78-101)
🔇 Additional comments (8)
packages/vue-i18n-core/test/composer.test-d.ts (3)
9-9: LGTM!The addition of
NumberOptionsimport is necessary for the updated type assertions.
349-364: Good fix for the type inference issue.The removal of the third generic parameter aligns with the PR objectives. The type system can now correctly infer the return type based on the
partoption without explicit type declarations conflicting with the inference.
367-388: Test updates correctly validate the conditional return types.The test assertions properly verify that:
- When
part: falseor not specified, the methods returnstring- When
part: true, the methods return the appropriate format parts arrayThis confirms the type inference is working as expected.
packages/vue-i18n-core/src/composer.ts (3)
245-246: Well-designed utility type for conditional return types.The
IsPart<O>type alias elegantly extracts thepartproperty type or defaults tofalse. This enables the conditional return type logic needed to fix the type inference issue.
1116-1150: Excellent implementation of the conditional return type.The changes correctly implement dynamic return types based on the
partoption:
- Removed the explicit
Returngeneric parameter that was causing conflicts- Uses
IsPart<typeof keyOrOptions>to conditionally return eitherIntl.DateTimeFormatPart[]whenpartis truthy orstringotherwise- Maintains backward compatibility while fixing the type inference issue
1212-1252: Consistent implementation with proper type constraints.The number formatting interface changes mirror the datetime formatting approach:
- Introduces
OptionsTypegeneric to capture the actual options type- Uses
IsPart<OptionsType>for conditional return types- Properly constrains the generic types for type safety
This ensures consistent behavior across both formatting APIs.
packages/vue-i18n/src/vue.d.ts (2)
29-30: Consistent type alias implementation.The
IsParttype alias is correctly duplicated here for the Vue module declarations, maintaining consistency with the core composer implementation.
673-740: Correct implementation of conditional return types for $n.The number formatting methods properly implement the conditional typing:
OptionsTypegeneric is correctly used in both type parameters and method parameters- Return types conditionally resolve based on the
partoption- Maintains consistency with the core composer implementation
Size ReportBundles
Usages
|
@intlify/core
@intlify/core-base
@intlify/devtools-types
@intlify/message-compiler
@intlify/shared
petite-vue-i18n
vue-i18n
@intlify/vue-i18n-core
commit: |
|
Welcome contribution! |
fix #2192
Hello 🖖
I don't know how to fix 2 failings existing tests
expectTypeOf( d<Date, string>(..., {..., part: true }) ).toEqualTypeOf<Intl.DateTimeFormatPart[]>()expectTypeOf( n<string>(..., {..., part: true }) ).toEqualTypeOf<Intl.NumberFormatPart[]>()Probably, declare the 2nd generic type as
stringis in conflict with{ part: true }type inference.The correct usage should be type inference, without generic declaration.
So in my opinion, like these tests was added in PR introducing regression, they are not correct, and can be removed.
Please could you confirm?
Summary by CodeRabbit
Summary by CodeRabbit
Refactor
partoption.Tests
Documentation