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(module): retain props reactivity through useUI #745

Merged
merged 14 commits into from
Sep 28, 2023

Conversation

aditio-eka
Copy link
Contributor

@aditio-eka aditio-eka commented Sep 27, 2023

πŸ”— Linked issue

#742

❓ 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

Resolves #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.

Read more

  • Use toRef to retain reactivity

If you really need to destructure the props, or need to pass a prop into an external function while retaining reactivity, you can do so with the toRefs() and toRef() utility APIs:

Read more

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@vercel
Copy link

vercel bot commented Sep 27, 2023

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

Copy link
Member

Awesome! Why did you mark this as breaking change?

@aditio-eka
Copy link
Contributor Author

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.

Copy link
Member

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

@benjamincanac benjamincanac changed the title feat(module): Retain props reactivity using toRef fix(module): retain props reactivity through useUI Sep 27, 2023
Copy link
Member

@benjamincanac benjamincanac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@aditio-eka
Copy link
Contributor Author

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

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

@aditio-eka
Copy link
Contributor Author

What type should we fix?

@benjamincanac
Copy link
Member

CleanShot 2023-09-27 at 17 11 50@2x

@aditio-eka
Copy link
Contributor Author

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.

Copy link
Member

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

aditio-eka commented Sep 28, 2023

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?

Copy link
Member

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

Copy link
Member

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

aditio-eka commented Sep 28, 2023

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?

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

@benjamincanac
Copy link
Member

benjamincanac commented Sep 28, 2023

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

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

@benjamincanac benjamincanac merged commit 109ec52 into nuxt:dev Sep 28, 2023
2 checks passed
Copy link
Member

Thanks for the PR! 😊

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

Successfully merging this pull request may close these issues.

The component class doesn't update during runtime changes when using the latest updates
2 participants