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 #3331

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

fix(components): improve generic types #3331

wants to merge 59 commits into from

Conversation

sandros94
Copy link
Member

@sandros94 sandros94 commented Feb 16, 2025

πŸ”— Linked issue

Resolves #2140
Also discussed privately and https://github.com/nuxt/ui-pro/pull/753#issuecomment-2625295701

❓ 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

The main goal for this PR is to both simplify the use of types as well as properly document them for each component that might use them.

Notes

  • I'm not adding type examples for some components, like Carousel, because the inferred string[] is more than enough.
  • Commit 5203359 might seem a breaking change (order of type generics), but it is how it should actually work.
  • CommandPalette ([CommandPalette] Generics not working well with items inside groupsΒ #2260) will require additional work in separate PR
  • Some components, like Breadcrumb have a labelKey?: string that I was looking for changing with proper inference, but for some reason they all break the forwarding of props

πŸ“ Components Checklist

As this PR touches basically almost all components here is a brief description on what changed for what component.

Legend

  • docs: means types have been added to doc (both markdown as well as vue component examples)
  • examples: means that types have been added only to related component example in docs
  • generics: means that I both added default types to generics as well as I might have reordered them (I don't consider this change a breaking change because it favors proper behavior)
  • dynamics: means that the component has been fully worked on and support proper dynamic typing based on input props (both arrays, nested ones, filters, modelValues, label/value keys)
  • *: means that I might need an additional verification before merge

Components

  • Accordion: docs, generics (ui-pro typecheck error, as expected)
  • Breadcrumb: docs, generics
  • ButtonGroup: examples
  • Calendar: generics
  • Carousel: generics
  • CommandPalette: probably will be covered in different PR
  • ContextMenu: docs, generics, dynamics
  • DropdownMenu: docs, generics, dynamics
  • Form: examples
  • InputMenu: examples, generics, dynamics
  • NavigationMenu: docs, generics, dynamics
  • RadioGroup: docs, generics *
  • Select: examples, generics, dynamics
  • SelectMenu: examples, generics, dynamics
  • Stepper: docs, generics
  • Table: probably will be covered in different PR
  • Tabs: docs, generics

Copy link

pkg-pr-new bot commented Feb 16, 2025

npm i https://pkg.pr.new/@nuxt/ui@3331

commit: 667b6ce

@sandros94
Copy link
Member Author

sandros94 commented Feb 17, 2025

Finally managed to fix dynamic typing!
This mostly affects InputMenu, Select and SelectMenu. This should also be the key to unblock the CommandPalette issue I was having, although I still think I will tackle it in a different PR.

Currently everything works for InputMenu:

  • dynamic typing with fetched content
    image
  • modelValue typing based on selected value-key (and multiple)
    image
    image
  • and ofc mixed multi-dimensional types
    image

@sandros94 sandros94 marked this pull request as ready for review February 18, 2025 13:33
@benjamincanac benjamincanac changed the title refactor: types generics, defaults and docs fix(components): improve generic types Feb 21, 2025
@sandros94 sandros94 marked this pull request as draft February 21, 2025 17:18
@sandros94

This comment was marked as outdated.

@sandros94
Copy link
Member Author

Update on the state of this PR

Dynamic Typing

I've standardized as much as possible the type utils that are responsible for dynamically typing modelValue, defaultValue, labelKey and valueKey. Although they can accept T | T[] | T[][], only when the input is an array (or nested) the returned type is dynamic and not simply T.
With dynamic typing I'm talking about all additional values that aren't the default ones the the component's item. For example:

const items: NavigationMenuItem[] = [
  {
    title: 'Home',
    to: '/',
    test: 1, // non-default type
  },
  {
    title: 'About',
    to: '/about',
  }
]

On this subject I would like to take the opportunity to standardize this one. ATM all the Reka UI components that accept items type with AcceptableValue mean that they all can support additional values, and I would like to keep this supported. But I would like to not simply add [key: string]: any, as it will not infer the type when, for example, accessing it via modelValue.

I already drafted and tested locally how to correctly support this with the following structure:

interface [COMPONENTNAME]ItemBase<K extends {}> {
  title?: string
  to?: string
  children?: MyItem<K>[]
}
type [COMPONENTNAME]Item<K extends {} = {}> = [COMPONENTNAME]ItemBase<K> & K

// used as follow for the example above
const items: NavigationMenuItem<{
  test?: number
}>[] = [
// ...
]

But now I would like to list out all those components that we want this to be supported. Some that come on top of my mind are:

  • Carousel
  • CommandPalette
  • InputMenu
  • NavigationMenu
  • RadioGroup
  • Select
  • SelectMenu
  • Table
  • Tabs
  • Tree (once it gets merged)

Slots

The current DynamicSlots utility type isn't working properly. It appears to be an easy fix (relatively speaking), but greatly depends on the type utils that are used for the dynamic typing. As such I will work on them (in this PR) after the above as been standardized fully.

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

Successfully merging this pull request may close these issues.

[NavigationMenu/DropdownMenu/ContextMenu] Generic type doesn't work when passing links as [][]
1 participant