unovue / shadcn-vue

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

[Bug]: Input `attrs` are not reactive #224

Closed ParasSolanki closed 9 months ago

ParasSolanki commented 10 months ago

Environment

Developement/Production OS: Windows 10 Pro 10.0.19045
Node version: 18.14.0
Package manager: pnpm@8.12.1
Radix Vue version: 1.2.5
Shadcn Vue version: 0.8.5
Vue version: 3.3.13
Nuxt version: 3.0.0
Nuxt mode: universal
Nuxt target: server
CSS framework: tailwindcss@3.4.0
Client OS: Windows 10 Pro 10.0.19045
Browser: Chrome 120.0.6099.130

Link to minimal reproduction

https://stackblitz.com/edit/8qmftg?file=src%2FApp.vue

Steps to reproduce

  1. Start a new Vue 3, Typescript project with Vite and create-vue pnpm create vue@latest.
  2. Install tailwindcss and its peer dependencies.
  3. After that Run the shadcn-vue init command to setup shadcn-vue in project. pnpm dlx shadcn-vue@latest init.
  4. Install Input component from shadcn-vue. pnpm dlx shadcn-vue@latest add input.
  5. Start the local dev server. pnpm run dev.
  6. Create basic form in App.vue component with Input component from shadcn-vue and create password input so when we click on it it changes the type attribute of input tag.
    
    <script setup lang="ts">
    import { ref } from 'vue';
    import { Input } from '@/components/ui/input';
    import { cn } from '@/lib/utils';
    import { EyeIcon, EyeOffIcon } from 'lucide-vue-next';

const showPassword = ref(false);


### Describe the bug

Yesterday, I was playing around with the `shadcn-vue` package for a little side project that I was building. I needed a password input for the signin/signup form. I added the input component from `shadcn-vue` and imported it into my form. I also needed a toggle button in the password input so that when the user clicks on it, it switches from type="password" to type="text" and vice versa. So, I implemented a basic form with a password input, and when I click on it, it toggles the `type`.

```vue
<script setup lang="ts">
import { ref } from 'vue';
import { Input } from '@/components/ui/input';
import { cn } from '@/lib/utils';
import { EyeIcon, EyeOffIcon } from 'lucide-vue-next';

const showPassword = ref(false);
</script>

<template>
  <form>
    ...
    <div class="relative">
      <Input
        placeholder="Enter password"
        :type="showMyPassword ? 'text' : 'password'"
      />
      <button
        type="button"
        class="absolute right-0 top-1/2 -translate-y-1/2 p-3"
        @click="showMyPassword = !showMyPassword"
      >
        <EyeIcon v-if="showMyPassword" class="h-4 w-4" />
        <EyeOffIcon v-else class="h-4 w-4" />
      </button>
    </div>
  </form>
</template>

But this was not working when i clicked on toggle button it was not toggling the type attribute on input. I was not able to figure out why its not working because the icon was changing but why type attribute is not changing?.

Then i checked the code of Input component and i found that we are using vue useAttrs() composable and grabbing all attributes destructuring the class and rest from it and adding to our input tag.

<script setup lang="ts">
import { useAttrs } from 'vue'
import { useVModel } from '@vueuse/core'
import { cn } from '@/utils'

defineOptions({
  inheritAttrs: false,
})

const props = defineProps<{
  defaultValue?: string | number
  modelValue?: string | number
}>()

const emits = defineEmits<{
  (e: 'update:modelValue', payload: string | number): void
}>()

const { class: className, ...rest } = useAttrs()  // here we are destructuring attrs

const modelValue = useVModel(props, 'modelValue', emits, {
  passive: true,
  defaultValue: props.defaultValue,
})
</script>

<template>
  <input v-model="modelValue" :class="cn('flex h-10 w-full rounded-md border border-input bg-background px-3 py-2 text-sm ring-offset-background file:border-0 file:bg-transparent file:text-sm file:font-medium placeholder:text-muted-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50', className ?? '')" v-bind="rest">
</template>

When we destructure the attrs we looses the reactivity and that's why the attribute was not changing.

Expected behavior

The Expected behavior is attrs should be reactive. When it changes it also updates the input attributes.

Solution

We can solve this by using computed property. Here is the code for it.

<script setup lang="ts">
import { useAttrs, computed } from "vue";
import { useVModel } from "@vueuse/core";
import { cn } from "~/lib/utils";

defineOptions({
  inheritAttrs: false,
});

const props = defineProps<{
  defaultValue?: string | number;
  modelValue?: string | number;
}>();

const emits = defineEmits<{
  (e: "update:modelValue", payload: string | number): void;
}>();

const rawAttrs = useAttrs();

// using computed property
const attrs = computed(() => {
  const { class: className, ...rest } = rawAttrs;

  return { className, rest };
});

const modelValue = useVModel(props, "modelValue", emits, {
  passive: true,
  defaultValue: props.defaultValue,
});
</script>

<template>
  <input
    v-model="modelValue"
    :class="cn('flex h-10 w-full rounded-md border border-input bg-background px-3 py-2 text-sm ring-offset-background file:border-0 file:bg-transparent file:text-sm file:font-medium placeholder:text-muted-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50', attrs.className ?? '')"
    v-bind="attrs.rest"
  />
</template>

Conext & Screenshots (if applicable)

Demo showing that after changing to computed property it is working.

shadcn-vue-input-demo.webm

sadeghbarati commented 10 months ago

Will check it. Thanks for the report

ParasSolanki commented 10 months ago

If you want i could create a PR to fix this issue.

sadeghbarati commented 10 months ago

Vue SFC Playground

$attrs or const wholeAttrs = useAttrs() are read-only Proxy(Object) destructuring it will lead it to.. 😭, and we have to turn them to reactive ones with computed or other Vue APIs, but this will double the process and we have no choice

to prevent duplicate classes I thought this way would work, actually, it works but on the first render and without reactivity afterward

const { class: className, ...restOfAttributes } = useAttrs()

since propsDestructure doesn't work with $attrs and useAttrs let's use class prop for and let $attrs handle other attributes I wished they were all props and propsDestructure wasn't experimental

image

ParasSolanki commented 10 months ago

Yeah since propsDestructure is still experimental we have to use computed to make it reactive.

Also i have notice that we are using useAttrs() in FormItem, PaginationEllipsis with destructuring so we have to make changes there as well?

a2kJW commented 9 months ago

I've found this error with Form. When used with Form, Input won't update its validation state. Solved it by removing

defineOptions({
  inheritAttrs: false,
})

from Input as it removes the reactivity and bindings from the component (as described in docs here) No clue if it has some unwanted side effects but personally didn't found any.

ParasSolanki commented 9 months ago

I've found this error with Form. When used with Form, Input won't update its validation state. Solved it by removing

defineOptions({
  inheritAttrs: false,
})

from Input as it removes the reactivity and bindings from the component (as described in docs here) No clue if it has some unwanted side effects but personally didn't found any.

It would be helpful if you can provide minimal reproduction (stackblitz/codesandbox). So we can check whats the issue.

a2kJW commented 9 months ago

I've found this error with Form. When used with Form, Input won't update its validation state. Solved it by removing

defineOptions({
  inheritAttrs: false,
})

from Input as it removes the reactivity and bindings from the component (as described in docs here) No clue if it has some unwanted side effects but personally didn't found any.

It would be helpful if you can provide minimal reproduction (stackblitz/codesandbox). So we can check whats the issue.

You can see the issue even in the docs under Form. Any form with validation, when the input validation fails, the aria-invalid always keeps the initial value. However, when using a normal html input it works correctly, the aria-invalid changes. I've noticed it while trying to change Input's style while invalid.

I can create a separate repo that recreates the issue but I don't think it's necessary here. You can even check it just with browser's inspection.

ParasSolanki commented 9 months ago

Yeah it is because we are destructuring attrs and we lose reactivity. When attributes change it does not reflect.

For the solution we have to define class inside props and bind other attributes without destructuring.

Here is the Solution you could use for now. This is latest commit with the same implementation soon it will be fix in live.

input-attrs-issue.webm

a2kJW commented 9 months ago

Yeah it is because we are destructuring attrs and we lose reactivity. When attributes change it does not reflect.

For the solution we have to define class inside props and bind other attributes without destructuring.

Here is the Solution you could use for now. This is latest commit with the same implementation soon it will be fix in live.

input-attrs-issue.webm

This works well, great work. Is there any reason why disable inheritAttrs?

ParasSolanki commented 9 months ago

Yeah it is because we are destructuring attrs and we lose reactivity. When attributes change it does not reflect. For the solution we have to define class inside props and bind other attributes without destructuring. Here is the Solution you could use for now. This is latest commit with the same implementation soon it will be fix in live. input-attrs-issue.webm

This works well, great work. Is there any reason why disable inheritAttrs?

Yeah because by default inheritAttrs will be true and it will merge attributes such as "class", "style" etc.. Here we are disabling attribute inheritance because otherwise it will merge attribute and it will be duplicated in input tag and we dont want that. So, by disabling we are taking control of it and binding attribute as we want.

You can take a look at this Vue SFC Playground to understand better.

I hope this explains. Sorry for my bad english.