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

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

Open
2 tasks
sadeghbarati opened this issue Aug 30, 2024 · 6 comments
Open
2 tasks
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Aug 30, 2024

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

  • I intend to submit a PR for this feature.
  • I have already implemented and/or tested this feature.
@sadeghbarati sadeghbarati added enhancement New feature or request help wanted Extra attention is needed labels Aug 30, 2024
@sadeghbarati sadeghbarati pinned this issue Aug 30, 2024
@sadeghbarati
Copy link
Collaborator Author

sadeghbarati commented Aug 30, 2024

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
Copy link
Collaborator Author

sadeghbarati commented Aug 30, 2024

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
Copy link
Collaborator

hrynevychroman commented Aug 30, 2024

@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
Copy link
Contributor

kikky7 commented Aug 30, 2024

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
Copy link
Contributor

Saeid-Za commented Sep 2, 2024

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
Copy link

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants