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 typings (extend vue instead of @vue/runtime-core) #69

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

felixgabler
Copy link
Contributor

Changes in PR:

Switched to extending vue instead of @vue/runtime-core as recommended in the official docs. Currently, it breaks type checking and throws a lot of type errors when this library is included.

https://vuejs.org/guide/extras/web-components#web-components-and-typescript

Also, it would be great if dependencies could be updated. @kyvg let me know if you'd like me to open another PR for it but I don't know if there are any restrictions or things to watch out for with this library.

@@ -1,6 +1,6 @@
import { notify } from './index';

declare module '@vue/runtime-core' {
declare module 'vue' {
export interface ComponentCustomProperties {
$notify: typeof notify;
}
Copy link

Choose a reason for hiding this comment

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

Code Review:

  1. Import Statement:

    • Make sure that the path to the notify function in the import statement (import { notify } from './index';) is correct and leads to the intended definition.
  2. Module Declaration:

    • The change in module declaration from '@vue/runtime-core' to 'vue' might be incompatible if the actual implementation relies on Vue 3's @vue/runtime-core. Ensure this change is intentional and won't break other parts of your application using Vue 3.
  3. Type Definition:

    • Verify that the $notify property is correctly added to the Vue instance for usage.
  4. Consistency:

    • Ensure consistency with other type/declaration files and conventions used in your project.
  5. Testing:

    • Test to ensure that the notifications work as expected after this change.
  6. Documentation:

    • Add comments or documentation to explain why this specific module declaration change was made for future reference.

Suggestions for Improvement:

  • If possible, provide more context on what notify does and how it should be handled within the Vue component.

Bug Risks:

  • The primary concern would be the potential breaking changes of module declaration if 'vue' doesn't incorporate the necessary types or interfaces you need to extend. Double-check compatibility with the version of Vue you are using.

By addressing these points, you can enhance the clarity, maintainability, and compatibility of the code.

@kyvg
Copy link
Owner

kyvg commented Aug 23, 2024

Thank you for your PR! I'll try to update dependencies this weekend

@kyvg kyvg merged commit 26dee01 into kyvg:master Aug 23, 2024
@felixgabler
Copy link
Contributor Author

Awesome! Looking forward to the next release

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.

2 participants