Skip to content

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

Merged
merged 5 commits into from
Jan 16, 2019
Merged

feat: TypeScript support #43

merged 5 commits into from
Jan 16, 2019

Conversation

potato4d
Copy link
Contributor

@potato4d potato4d commented Dec 31, 2018

Changes

  1. Install TypeScript to dev dependencies
  2. Create tsconfig.js copied from https://github.com/vuejs/vue/blob/dev/types/tsconfig.json
  3. Define types

Copy link
Member

@kazupon kazupon left a 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'
Copy link
Member

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

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

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

Copy link
Member

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.

@potato4d
Copy link
Contributor Author

@kazupon @ktsn
Thank you for review!
I finished improve it.

types/index.d.ts Outdated
@@ -0,0 +1,10 @@
import _Vue, { AsyncComponent, ComponentOptions } from 'vue'
Copy link
Member

@HerringtonDarkholme HerringtonDarkholme Jan 16, 2019

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.

Copy link
Contributor Author

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 🙏

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! 😢

Copy link
Contributor Author

@potato4d potato4d Jan 16, 2019

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

Choose a reason for hiding this comment

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

Yes, it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HerringtonDarkholme resolve it!

Copy link
Member

@ktsn ktsn left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

@kazupon
Copy link
Member

kazupon commented Jan 16, 2019

@potato4d Thanks!
@ktsn @HerringtonDarkholme
Thank you for your reviewing!

@kazupon kazupon merged commit e2b8456 into vuejs:master Jan 16, 2019
Comment on lines +3 to +6
declare function wrap(
Vue: typeof _Vue,
Component: Component | AsyncComponent
): HTMLElement

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().)

Copy link

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.

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.

6 participants