Skip to content
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

fix(components): improve generic types #2499

Draft
wants to merge 22 commits into
base: v3
Choose a base branch
from

Conversation

yassilah
Copy link
Contributor

@yassilah yassilah commented Oct 31, 2024

🔗 Linked issue

Replaces #2490, #2491, #2482

Resolves #2260, resolves #2140

This PR is a full refactor of dynamic slots inside components with improved generic types, dynamic slot names, custom properties, arrays of arrays, etc. It includes type tests for all of the components to check the payload of each slot and infer available slot names based on props.

It also replaces the previous DynamicSlots type helper with a new one:

type DynamicSlots<
    Items,  // list of items that may contain a `SlotKey`
    Suffix, // list of suffixes to add to the generated slots (e.g. `leading` or `trailing`)
    StaticKeys, // list of static slot keys to add (e.g. `item` or `content`)
    SlotKey // the key where the slot name of each item should be found
> = ...

type SlotDef = DynamicSlots<[{ slot: 'foo', value: 'cool' }], 'leading' | 'trailing', 'item', 'slot'> 

// {
//    item: { slot: 'foo', value: 'cool' },
//    item-leading: { slot: 'foo', value: 'cool' },
//    item-trailing: { slot: 'foo', value: 'cool' },
//    foo: { slot: 'foo', value: 'cool' },
//    foo-leading: { slot: 'foo', value: 'cool' },
//    foo-trailing: { slot: 'foo', value: 'cool' }
// }

const slots = defineSlots<{ [K in keyof SlotDef]: (params: SlotDef[K]) => any  }>()

❓ Type of change

  • 📖 Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

📝 Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@yassilah yassilah changed the title fix(DropdownMenu/ContextMenu/CommandPalette/NavigationMenu/Accordion/Breadcrumb): improve generic types fix(DropdownMenu/ContextMenu/CommandPalette/NavigationMenu/Accordion/Breadcrumb/Tabs): improve generic types Oct 31, 2024
Copy link

pkg-pr-new bot commented Oct 31, 2024

pnpm add https://pkg.pr.new/@nuxt/ui@2499

commit: b308d42

@benjamincanac benjamincanac changed the title fix(DropdownMenu/ContextMenu/CommandPalette/NavigationMenu/Accordion/Breadcrumb/Tabs): improve generic types fix(components): improve generic types Oct 31, 2024
@benjamincanac
Copy link
Member

benjamincanac commented Oct 31, 2024

Thanks a lot for this! After some tests, I don't have autocomplete on custom slots for NavigationMenu, DropdownMenu, ContextMenu, etc. I think there's also the default slot missing for Accordion.

(I've updated the PR to add the missing item-label slot on NavigationMenu)

@@ -55,8 +56,52 @@ describe('Accordion', () => {
['with body slot', { props: { ...props, modelValue: '1' }, slots: { body: () => 'Body slot' } }],
['with custom slot', { props: { ...props, modelValue: '5' }, slots: { custom: () => 'Custom slot' } }],
['with custom body slot', { props: { ...props, modelValue: '5' }, slots: { 'custom-body': () => 'Custom body slot' } }]
])('renders %s correctly', async (nameOrHtml: string, options: { props?: AccordionProps<typeof items[number]>, slots?: Partial<AccordionSlots<typeof items[number]>> }) => {
])('renders %s correctly', async (nameOrHtml: string, options: { props?: AccordionProps<typeof items>, slots?: Partial<AccordionSlots<typeof items>> }) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this change not applied everywhere? 🤔

@yassilah
Copy link
Contributor Author

Hey so i just tried and autocomplete does work but there's one test case I haven't tried which indeed doesn't work: when a slot is set in one item but not in the others, e.g.:

Capture d’écran 2024-10-31 à 12 09 24

VS

Capture d’écran 2024-10-31 à 12 10 53

I think I know why, I'll try to fix it

body: SlotProps<T>
} & DynamicSlots<T, SlotProps<T>>
export type AccordionSlots<Items> =
(DynamicSlots<Items, null, 'trailing' | 'leading' | 'body' | 'content'> extends infer S ? { [K in keyof S]: SlotProps<S[K]> } & Record<string, any> : never) & (DynamicSlots<Items, 'body', null> extends infer S ? { [K in keyof S]: SlotProps<S[K]> } & Record<string, any> : never)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there two definition for body? 🤔

@yassilah
Copy link
Contributor Author

Ok, so actually now I'm wondering if this is really a better DX as to really get the benefits you have to use as const satisfies MyInterface[] or as const satisfies MyInterface[][] and the implementation is a bit tedious. I'm actually suggesting another option that would make things a lot easier and provide a better overall DX I think:

Capture d’écran 2024-10-31 à 13 31 30 Capture d’écran 2024-10-31 à 13 31 20

@yassilah
Copy link
Contributor Author

yassilah commented Oct 31, 2024

Implementation would look like this and would have an extremely limited impact on the bundle since each function is just an alias. It could be auto-imported by Nuxt and then it'd be super easy to get all type hints and it would of course still be possible to not use these helpers at all

Capture d’écran 2024-10-31 à 13 35 00

Anyway that's just a suggestion and maybe that's too overkill aha

@benjamincanac
Copy link
Member

benjamincanac commented Oct 31, 2024

I'm not a fan honestly of this. Do we really need to do as const satisfies ...? Is it required for [][] items only?

I have the right autocompletion for the Accordion without it:
CleanShot 2024-10-31 at 14 22 03@2x

And it works with only one item having the slot field defined.

@yassilah
Copy link
Contributor Author

yassilah commented Nov 3, 2024

I'm not a fan honestly of this. Do we really need to do as const satisfies ...? Is it required for [][] items only?

I have the right autocompletion for the Accordion without it: CleanShot 2024-10-31 at 14 22 03@2x

And it works with only one item having the slot field defined.

With the current PR? and without using any as const? You don't need to use satisfies ... but it is required if you want to get autocomplete while writing your list of items to know what properties AccordionItem can have

Copy link
Member

I thought you were talking about slot name autocomplete.

To know how you can define items, I'd just do const items: NavigationMenuItem[] = [].

Is this PR ready to merge?

@yassilah
Copy link
Contributor Author

yassilah commented Nov 3, 2024

Yes I was talking about the slot name autocomplete, how can you have it without specifying that it's a constant? Anyway, I was just going through the PR and actually I think i overly complicated things, let me have another go at it 😅 sorry for the going back and forth

@benjamincanac benjamincanac added the v3 #1289 label Nov 5, 2024
@benjamincanac benjamincanac marked this pull request as draft November 5, 2024 21:26
@benjamincanac
Copy link
Member

@yassilah Should I close this? Do you plan to start over or improve this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v3 #1289
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants