fix(module): retain props reactivity through `useUI`

aditio-eka commented 8 months ago

🔗 Linked issue

https://github.com/nuxt/ui/issues/742

❓ Type of change

[ ] 📖 Documentation (updates to the documentation or readme)
[x] 🐞 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

Resolves https://github.com/nuxt/ui/issues/742. There are two main changes:

Use the prop for class instead of the fallthrough attribute because the fallthrough attribute isn't reactive.

Note that although the attrs object here always reflects the latest fallthrough attributes, it isn't reactive (for performance reasons). You cannot use watchers to observe its changes. If you need reactivity, use a prop. Alternatively, you can use onUpdated() to perform side effects with the latest attrs on each update.

📝 Checklist

[x] I have linked an issue or discussion.
[ ] I have updated the documentation accordingly.

vercel[bot] commented 8 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
ui	✅ Ready (Inspect)	Visit Preview	Sep 28, 2023 11:58am

benjamincanac commented 8 months ago

Awesome! Why did you mark this as breaking change?

aditio-eka commented 8 months ago

Awesome! Why did you mark this as breaking change?

Because I encountered an error when trying to run pnpm dev, I guess it's caused by the new arguments of useUI. When I saw the error message, it seemed like some components in @nuxt/ui-pro are using useUI. We should update these components to match the new arguments of useUI.

benjamincanac commented 8 months ago

Oh yes indeed, I'll update @nuxt/ui-pro with this PR to fix the typecheck!

aditio-eka commented 8 months ago

I was wondering, since you defined class as string, this prevents the use of arrays and objects. Does this mean we have to implement what Vue does by default? https://vuejs.org/guide/essentials/class-and-style.html#binding-html-classes

I tested assigning objects and arrays to the class prop, and it transformed them into strings automatically.

benjamincanac commented 8 months ago

Ok, so we just need to fix the types right?

aditio-eka commented 8 months ago

What type should we fix?

benjamincanac commented 8 months ago

aditio-eka commented 8 months ago

I see, I didn't encounter a type error when I tested it in the playground. Maybe my VS Code has an issue. Yes, we should update class type. Do you have any suggestions on how to implement it? I'm not familiar with using runtime declarations; I usually use generic type arguments.

benjamincanac commented 8 months ago

I guess we can define the class prop as such:

class: {
  type: [String, Object, Array] as PropType<string | Record<string, unknown> | Array<string>>,
  default: undefined
}

aditio-eka commented 8 months ago

I guess we can define the class prop as such:

class: {
  type: [String, Object, Array] as PropType<string | Record<string, unknown> | Array<string>>,
  default: undefined
}

I just updated the class prop type using unknown. I followed the type from Vue in here. What do you think, @benjamincanac ? Should we define the type explicitly?

benjamincanac commented 8 months ago

Looks good! I'll finish the update of @nuxt/ui-pro so we can merge this 😊

benjamincanac commented 8 months ago

Actually, it might be best to define those with any:

    class: {
      type: [String, Object, Array] as PropType<any>,
      default: undefined
    },

Otherwise, I get TS errors once module is built. CleanShot 2023-09-28 at 12.39.45@2x.png What do you think?

aditio-eka commented 8 months ago

Actually, it might be best to define those with any:
    class: {
      type: [String, Object, Array] as PropType<any>,
      default: undefined
    },
Otherwise, I get TS errors once module is built. What do you think?

I think it's okay since the class prop is normalized by Vue.

benjamincanac commented 8 months ago

@aditio-eka Just made this so we can pass the config as a computed: fc321c8 (#745)

Should we update the T type or is it fine?

aditio-eka commented 8 months ago

I guess we can update the $config type of useUI to Ref<T> | T.

benjamincanac commented 8 months ago

Thanks for the PR! 😊

nuxt / ui