radix-vue / shadcn-vue

Vue port of shadcn-ui
https://www.shadcn-vue.com/
MIT License
4.38k stars 251 forks source link

[Feature]: Update Components to Vue 3.5 Reactive Props Destructure #742

Open sadeghbarati opened 2 weeks ago

sadeghbarati commented 2 weeks ago

Describe the feature

<script setup lang="ts">
import { type HTMLAttributes, computed } from 'vue'
import { AccordionItem, type AccordionItemProps, useForwardProps } from 'radix-vue'
import { cn } from '@/lib/utils'

- const props = defineProps<AccordionItemProps & { class?: HTMLAttributes['class'] }>()
+ const { class: className, ...props } = defineProps<AccordionItemProps & { class?: HTMLAttributes['class'] }>()

- const delegatedProps = computed(() => {
-  const { class: _, ...delegated } = props
- return delegated
- })

- const forwardedProps = useForwardProps(delegatedProps)
+ const forwardedProps = useForwardProps(props)
</script>

<template>
  <AccordionItem
    v-bind="forwardedProps"
-    :class="cn('border-b', props.class)"
+    :class="cn('border-b', className)"
  >
    <slot />
  </AccordionItem>
</template>

Additional information

sadeghbarati commented 2 weeks ago

I really don't like to reassign class: className 🙅

But If I use only class I'll get this error

Screenshot 2024-08-30 at 10 51 27 PM

@zernonia is this intended? should we ask it in Vue Language Tools repo about this? Sorry this is eslint error, but how to remove this eslint rule? It has runtime error check next Stackblitz please (vue-ts starter template)

sadeghbarati commented 2 weeks ago
Screenshot 2024-08-30 at 11 52 44 PM

https://stackblitz.com/edit/vitejs-vite-6eb22f?file=vite.config.ts,src%2Fcomponents%2FHelloWorld.vue&terminal=dev

Is this a reason why we have something like className? 😶‍🌫️

How we can use destructured class prop without reassign it to className or any other names

hrynevychroman commented 2 weeks ago

@sadeghbarati I think one working approach will be { class: cl } or something like this. This is not an ESLint error, this is a JavaScript (TS) preserved variable name:

How can we omit this issue:

image

Default error:

image

Reserved words documentation link: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#reserved_words

image
kikky7 commented 2 weeks ago

Yeah, you can't name your variable class, that's why className is used. Vue props have different scope so you can name it class in there.

I usually name it cls or _class or something like that, className is too long and too reacty for me, but anyway you need to name it somehow, if you use such approach, because class is reserved word in JS.

Saeid-Za commented 1 week ago

I support the proposed change, but it is important to note that this is a breaking change. Anyone using Vue versions earlier than 3.5 without enabling the experimental flag may encounter unexpected errors.

In my experience, there may not be an explicit error message, but the reactivity will fail to function properly, making it difficult to debug.

If this is to be implemented, we would need to properly communicate the required migration steps.

Dino-Kupinic commented 6 days ago

Like @Saeid-Za said, this is a breaking change and imo most people would rather appreciate bug fixes and more mirroring with the official shadcn (new charts)