-
Notifications
You must be signed in to change notification settings - Fork 102
feat: TypeScript support #43
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ping @ktsn @HerringtonDarkholme
Could you review this PR?
types/index.d.ts
Outdated
@@ -0,0 +1,8 @@ | |||
import * as VueJS from 'vue' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To adjust to the type definitions of other libraries, I think that we should be directly import rather than as
using.
import _Vue from 'vue'
ref: vuex https://github.com/vuejs/vuex/blob/dev/types/index.d.ts#L1
types/index.d.ts
Outdated
import * as VueJS from 'vue' | ||
|
||
declare function wrap( | ||
Vue: VueJS.default, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the above, I think that it will be below using typeof
.
Vue: typeof _Vue
types/index.d.ts
Outdated
|
||
declare function wrap( | ||
Vue: VueJS.default, | ||
Component: VueJS.VueConstructor<VueJS.default> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the asynchronous component, i think we should be defined the below.
ComponentOptions<Vue> | typeof Vue | AsyncComponent
ref: vue-router
https://github.com/vuejs/vue-router/blob/dev/types/router.d.ts#L3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Component | AsyncComponent
may be better, I guess. It includes functional component and the type parameter of ComponentOption
is never
which makes it the top type in type parameters variation as the type param is contravariant.
types/index.d.ts
Outdated
@@ -0,0 +1,10 @@ | |||
import _Vue, { AsyncComponent, ComponentOptions } from 'vue' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~~ I think we already have exported Component
from vue. Can we use it? https://github.com/vuejs/vue/blob/dev/types/index.d.ts#L13 ~~
@ktsn has already pointed that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HerringtonDarkholme
Sorry, I was misreading.
I corresponded. thank you for review 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget to include AsyncComponent
, sorry! 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HerringtonDarkholme
Is the following code correct?
declare function wrap(
Vue: typeof _Vue,
Component: Component | AsyncComponent
): HTMLElement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HerringtonDarkholme resolve it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM.
@potato4d Thanks! |
declare function wrap( | ||
Vue: typeof _Vue, | ||
Component: Component | AsyncComponent | ||
): HTMLElement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @potato4d,
I'm wondering if the return type here is correct.
(I'm expecting something like CustomElementConstructor
so it can be passed to CustomElementRegistry.define()
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed wrong , it should be CustomElementConstructor.
Changes