fix(system-rsc): extendVariants Fix/compound variants types #5847
Conversation
🦋 Changeset detectedLatest commit: d18110f The changes in this PR will be included in the next version bump. This PR includes changesets to release 37 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 |
|
@ITBoomBKStudio is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughUpdates TypeScript typings to make compound variants slots-aware by adding a third generic to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (5)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/system-rsc/src/extend-variants.d.ts (2)
51-54: CompoundVariants slots support looks good; consider a small readability tweak.You can express the value type via the existing helper to reduce repetition.
Apply:
type CompoundVariants<V, SV, S> = Array< VariantValue<V, SV> & - ClassProp<S extends undefined ? ClassValue : ClassValue | SlotsClassValue<S>> + ClassProp<ClassValue | GetSuggestedValues<S>> >;
98-99: Use the already-bound S instead of recomputing ComponentSlots.Keeps generics consistent and may help inference.
- CV extends CompoundVariants<V, SV, ComponentSlots<CP>>, + CV extends CompoundVariants<V, SV, S>,
Replaces the conditional ClassProp logic with a simpler, consistent form to fix incorrect slot value inference. Before: ClassProp<S extends undefined ? ClassValue : ClassValue | SlotsClassValue<S>> After: ClassProp<ClassValue | GetSuggestedValues<S>> This ensures GetSuggestedValues<S> is used for slot-aware variants and avoids duplicated conditional branches for undefined slots.
I’m keeping CV extends CompoundVariants<V, SV, ComponentSlots<CP>> on purpose. |
ITBoomBKStudio
changed the title
ITBoomBKStudio
changed the title
…NonNullable Simplifies the return type of ExtendVariants to ensure no required props are enforced at the HOC level. This aligns with the intended API contract where extended components expose all props as optional. - All keys (CP ∪ V) are optional - Preserve CP type hints and booleanized V values - Added NonNullable<V[key]> guard to prevent undefined indexing
|
Added the final compoundVariants test - all cases now covered and passing |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/system-rsc/__tests__/extend-variants.test.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/system-rsc/__tests__/extend-variants.test.tsx (4)
packages/core/system-rsc/src/extend-variants.js (1)
styles(105-105)packages/core/system-rsc/src/extend-variants.d.ts (1)
ExtendVariantWithSlotsProps(76-80)packages/core/system/src/index.ts (1)
ExtendVariantWithSlotsProps(16-16)packages/core/system-rsc/src/index.ts (1)
ExtendVariantWithSlotsProps(29-29)
🔇 Additional comments (3)
packages/core/system-rsc/__tests__/extend-variants.test.tsx (3)
1-1: LGTM!The import of
ExtendVariantWithSlotsPropsis necessary to support the updated type signature for the slots-based test helper and aligns with the PR's objective to expose slots-aware compound variant types.
40-40: LGTM!The parameter type change from
ExtendVariantPropstoExtendVariantWithSlotsPropscorrectly reflects that slots-based components require compound variants to support per-slot class definitions (e.g.,class: { header: "..." }), which is demonstrated in the new test at lines 274-276.
264-292: Well-structured test for slots-aware compound variants.This test effectively validates both forms of compound variant classes:
- String class applied to the base slot (line 270)
- Record class applied to specific slots like
header(lines 274-276)The test aligns well with the PR objective to ensure proper type inference for slots-aware compound variants.
However, the same concern applies here: both compound variants reference
radius: "md"(lines 269, 273), which is not defined in the extendedradiusvariants. If this is intentional to test compound variants referencing base component variants not overridden in the extension, consider adding a comment explaining this edge case.
@heroui/accordion
@heroui/alert
@heroui/autocomplete
@heroui/avatar
@heroui/badge
@heroui/breadcrumbs
@heroui/button
@heroui/calendar
@heroui/card
@heroui/checkbox
@heroui/chip
@heroui/code
@heroui/date-input
@heroui/date-picker
@heroui/divider
@heroui/drawer
@heroui/dropdown
@heroui/form
@heroui/image
@heroui/input
@heroui/input-otp
@heroui/kbd
@heroui/link
@heroui/listbox
@heroui/menu
@heroui/modal
@heroui/navbar
@heroui/number-input
@heroui/pagination
@heroui/popover
@heroui/progress
@heroui/radio
@heroui/ripple
@heroui/scroll-shadow
@heroui/select
@heroui/skeleton
@heroui/slider
@heroui/snippet
@heroui/spacer
@heroui/spinner
@heroui/switch
@heroui/table
@heroui/tabs
@heroui/toast
@heroui/tooltip
@heroui/user
@heroui/react
@heroui/system
@heroui/system-rsc
@heroui/theme
@heroui/use-aria-accordion
@heroui/use-aria-accordion-item
@heroui/use-aria-button
@heroui/use-aria-link
@heroui/use-aria-modal-overlay
@heroui/use-aria-multiselect
@heroui/use-aria-overlay
@heroui/use-callback-ref
@heroui/use-clipboard
@heroui/use-data-scroll-overflow
@heroui/use-disclosure
@heroui/use-draggable
@heroui/use-form-reset
@heroui/use-image
@heroui/use-infinite-scroll
@heroui/use-intersection-observer
@heroui/use-is-mobile
@heroui/use-is-mounted
@heroui/use-measure
@heroui/use-pagination
@heroui/use-real-shape
@heroui/use-ref-state
@heroui/use-resize
@heroui/use-safe-layout-effect
@heroui/use-scroll-position
@heroui/use-ssr
@heroui/use-theme
@heroui/use-update-effect
@heroui/use-viewport-size
@heroui/aria-utils
@heroui/dom-animation
@heroui/framer-utils
@heroui/react-rsc-utils
@heroui/react-utils
@heroui/shared-icons
@heroui/shared-utils
@heroui/stories-utils
@heroui/test-utils
commit: |
… tests for slots component
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/system-rsc/__tests__/extend-variants.test.tsx (1)
76-88: Default compound variant fix addresses previous review.The change from
radius: "md"toradius: "none"correctly uses a value from the extended variant set. The fallback logic withstyles?.compoundVariants ?? [...]is clean and well-structured.Optional: Consider adding test coverage for the first compound variant.
The default compound variant at lines 77-81 (
shadow: "none"+radius: "none") is never explicitly tested. The test at line 270-284 usesradius: "sm", so only the second compound variant (lines 82-87) is exercised. Adding a test case withradius="none" shadow="none"would verify that the fix for the previous review comment works correctly.Example test:
test("should apply compound variant when shadow and radius are none", () => { const Card2 = createExtendSlotsComponent(); const {getByTestId} = render( <Card2 radius="none" shadow="none"> Card Content </Card2>, ); const baseEl = getByTestId("base"); expect(baseEl).toHaveClass("rounded-sm"); // from compound variant, overriding radius="none"'s "rounded-xs" });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/system-rsc/__tests__/extend-variants.test.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/system-rsc/__tests__/extend-variants.test.tsx (2)
packages/core/system-rsc/src/extend-variants.js (1)
styles(105-105)packages/core/system-rsc/src/extend-variants.d.ts (1)
ExtendVariantWithSlotsProps(76-80)
🔇 Additional comments (3)
packages/core/system-rsc/__tests__/extend-variants.test.tsx (3)
1-1: LGTM! Import addition aligns with slots-aware type improvements.The new
ExtendVariantWithSlotsPropstype is correctly imported and used on line 40 to support compound variants with slot-specific styling.
40-40: Correct type change for slots-aware compound variants.Using
ExtendVariantWithSlotsPropsenables the test helper to accept compound variants with slot-specific class values, matching the updated type system.
270-314: Excellent test coverage for compound variants.The two new test cases correctly verify:
- Default compound variants work alongside regular variants (lines 270-284)
- Custom compound variants passed via
stylesparameter override defaults (lines 286-314)The tests validate both base-level and slot-specific compound variant application, ensuring the slots-aware type improvements function correctly at runtime.
ITBoomBKStudio
changed the title
Replace PropsWithoutRef with explicit Exclude<'ref'> in mapped keys and intersect with RefAttributes<InferRef<C>>. This prevents @types/react’s internal UNDEFINED_VOID_ONLY from leaking into the public .d.ts and fixes declaration emit for components like extended Autocomplete.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/system-rsc/src/extend-variants.d.ts (1)
103-107: Verify that making all keys optional is the intended behavior.The current mapped type uses
?:on line 104, making all keys optional—including those that may be required in the base component propsCP. A past review raised this concern and suggested preservingCP's original optionality by separating CP keys (which would retain their required/optional status) from V-only keys (which would be optional).That review was marked as addressed by updating tests to provide required props. If the intention is for extended components to always have optional props, the current implementation is correct. However, if base component contracts should be preserved (required props remain required), consider this alternative:
{ // CP keys preserve their original optionality [K in keyof CP]: K extends keyof V ? CP[K] | StringToBoolean<keyof NonNullable<V[K]>> : CP[K] } & { // V-only keys are optional [K in Exclude<keyof V, keyof CP>]?: StringToBoolean<keyof NonNullable<V[K]>> } & RefAttributes<InferRef<C>>This would maintain stricter type-safety by preventing accidental omission of required base props while still allowing variant props to be optional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/system-rsc/src/extend-variants.d.ts(4 hunks)
🔇 Additional comments (3)
packages/core/system-rsc/src/extend-variants.d.ts (3)
2-2: LGTM! Missing import added.The
JSXElementConstructorimport is correctly added and used in theComponentPropshelper (line 12) andExtendVariantsconstraint (line 86).
46-48: Slots-aware compound variants implemented correctly.The addition of the
Sgeneric parameter and use ofGetSuggestedValues<S>properly extends compound variants to support both plain class values and slot-keyed class values, maintaining backward compatibility when slots are undefined.
92-92: Appropriate use ofComponentSlots<CP>for broader type coverage.Using
ComponentSlots<CP>instead of the bound genericSensures compound variants have access to the full slot surface of the base component, maintaining type-safety and authoring ergonomics as noted in the PR objectives.
Yes, this behavior is intentional for components created with extendVariants. extendVariants is designed as a styling HOC, not a strict type-safe wrapper. If we preserved CP’s required props, every extended component instance would force full prop specification, which defeats the purpose of variant composition. |
… for consistent variant typing
|
Hey @jrgarciadev, both #5847 and #5848 are ready. They fix system-rsc type & slot inference issues. Would be great to get your review when you have a moment 👀 |
|
@wingkwong please check this PR |
There was a problem hiding this comment.
- The changes you added in
packages/core/system-rsc/__tests__/extend-variants.test.tsxdon't really handle the reported issue. i.e If I just run those new tests in canary branch, they are all passed. - Please merge your PR 5848 changes into this PR so that I could test them in one go.
- Please also handle the following cases.
The type error is on to.
export const CustomButton = extendVariants(Button, {});
<Button as={Link} to="/sign-in">
Sign In
</Button>
The type error is on defaultVariants.classNames.tabList
const CustomTabs = extendVariants(Tabs, {
slots: {
tabList: 'bg-white p-0',
base: 'w-full bg-white rounded-lg overflow-hidden',
tab: 'py-6',
tabContent: 'font-medium text-gray-800',
},
defaultVariants: {
variant: 'underlined',
color: 'primary',
classNames: {
tabList: 'bg-white p-0',
base: 'w-full bg-white rounded-lg overflow-hidden',
tab: 'py-6',
tabContent: 'font-medium text-gray-800',
},
},
});
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@heroui/system-rsc": major | |||
4 participants
Closes #5778
📝 Description
Supersedes #5795.
Builds on the initial work by @IsDyh01 to address the incorrect return type in
extendVariants, and additionally fixes long-standing typing issues with CompoundVariants (observed since early NextUI days).This PR:
extendVariantstype fix.Credits:
Original approach by @IsDyh01 (commits preserved via cherry-pick / attribution).
Authored by @ITBoomBKStudio.
⚽️ Current behavior (updates)
extendVariantscan yield an incorrect component return type, leading to type errors when using the extended variant API.🚀 New behavior
extendVariantsnow preserves and exposes the correct component/props type in the returned value.CompoundVariants inference is stable when:
asneeded).Added unit tests covering:
extendVariants,No runtime changes; all changes are type-level and test-only.
💣 Is this a breaking change (Yes/No):
No.
The change improves type inference without removing or renaming public APIs. Existing correct usages continue to type-check. In cases where code previously relied on loose/incorrect inference, the compiler now catches mistakes earlier (desired tightening, not a breaking API change).
📝 Additional Information
Summary by CodeRabbit