-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
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
Improve Vue type declarations for more canonical usage #5887
Conversation
… as members from 'data' and the Vue instance.
…Vue instance members.
…s, got rid of AnyVue.
Conflicts: types/vue.d.ts
Conflicts: types/options.d.ts types/test/options-test.ts types/test/vue-test.ts types/vue.d.ts
Thanks @DanielRosenwasser ! Are there any remaining incompatibilities aside from the need for using latest dependancies, e.g. required change in Also this would cover #5016 as far as I can tell. |
hmmm, the test is failing. It seems So interface augmentation fails. |
propsData?: Object; | ||
computed?: { [key: string]: ((this: V) => any) | ComputedOptions<V> }; | ||
methods?: { [key: string]: (this: V, ...args: any[]) => any }; |
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.
We might also need methods?: ThisType<Methods & Computed & Pops & Data>
to fully capture Vue's this
instance.
I haven't try it myself, but will it cause cyclic reference?
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 haven't try it myself, but will it cause cyclic reference?
There are a few problems that potentially come up if you try to reference methods
from data
in which you'll have to have an explicit annotation on data()
. But we can be prescriptive and tell people to just use functions when you need to operate on data
in some way.
We might also need methods?: ThisType<Methods & Computed & Pops & Data> to fully capture Vue's this instance.
@HerringtonDarkholme that's not actually necessary. When a method needs its this
type, it will walk up each contextually typed object literal until it finds one that had a contextual type consisting of ThisType
. If it does, it uses it. If not, it uses the innermost object literal's contextual type, and if there isn't one it just uses the type of the containing literal.
You should definitely try it 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.
Tried it locally. It works like charm!
Sorry for the wrong reporting. I only tried accurateVueType
in vetur, which seems to wrongly configured. I will investigate more in vetur.
Vetur's issue is caused by ComponentOption
's type arg.
@yyx990803 the fact that |
The build should now be fixed. |
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 awesome work! 👍
I badly wanted to use Vue.js with TypeScript on the native syntax 😄
BTW, we may also need to update vue-server-renderer types since it's depends on the core types.
types/vue.d.ts
Outdated
|
||
export type CombinedVueInstance<Data, Methods, Computed, Props> = Data & Methods & Computed & Props & Vue; | ||
export type ExtendedVue<Constructor extends VueConstructor, Data, Methods, Computed, Props> = | ||
(new (...args: any[]) => CombinedVueInstance<Data, Methods, Computed, Props>) & |
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.
As ExtendedVue
is a sub-class of the Vue constructor, we can expect the constructor signature would only receive one argument like below?
new (options: any) => CombinedVueInstance<Data, Methods, Computed, Props>
Not sure whether we could annotate the options
with ThisTypedComponentOptionsWithArrayProps
etc. since the type declaration might be so complicated 😂
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.
As ExtendedVue is a sub-class of the Vue constructor, we can expect the constructor signature would only receive one argument like below?
@ktsn ExtendedVue creates an intersection with the mixin constructor (new (...args: any[]) => CombinedVueInstance<Data, Methods, Computed, Props>)
. This means it just absorbs the argument types of the Constructor
type that's given. See microsoft/TypeScript#13743 for more details
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.
Oh, I didn't understand about the mixin constructor correctly. Thank you for the explanation.
types/vue.d.ts
Outdated
extend<VC extends VueConstructor, PropNames extends string = never>(this: VC, definition: FunctionalComponentOptions<PropNames[], Record<PropNames, any>>): ExtendedVue<VC, {}, {}, {}, Record<PropNames, any>>; | ||
extend<VC extends VueConstructor, Props extends Record<string, PropValidator>>(this: VC, definition: FunctionalComponentOptions<Props, Record<keyof Props, any>>): ExtendedVue<VC, {}, {}, {}, Record<keyof Props, any>>; | ||
extend<VC extends VueConstructor, Data, Methods, Computed, PropNames extends string = never>(this: VC, options: ThisTypedComponentOptionsWithArrayProps<Data, Methods, Computed, PropNames>): ExtendedVue<VC, Data, Methods, Computed, Record<PropNames, any>>; | ||
extend<VC extends VueConstructor, Data, Methods, Computed, Props extends Record<string, PropValidator>>(this: VC, options?: ThisTypedComponentOptionsWithRecordProps<Data, Methods, Computed, Props>): ExtendedVue<VC, Data, Methods, Computed, Record<keyof Props, any>>; |
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.
In the following code, this
type in Child
component options does not seem to have parent's properties while the new
ed Child
component has it.
const Parent = Vue.extend({
data () {
return { parent: 'Hello' }
}
})
const Child = Parent.extend({
methods: {
foo () {
this.parent // Compile error
}
}
})
const child = new Child()
child.parent // No error
But I'm not sure how we can solve this. It may need more type parameters to extract the parent instance types into ThisType
of the child component options....
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.
Good catch! I think I can fix this, but I'll have to look into it later today.
I just sent out a fix. In the previous iteration, |
@yyx990803 I think I could undo the commit if you don't want to fix #5016 right away. |
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 again for the awesome work! It really push Vue's typing to the horizon of TypeScript!
Since this definition is very sophisticated, it is even nicer to capture compiling error in test as well as compiling success. So we can ensure our definition correctly reject wrong code. In TS repo we have baseline test. In other langauges, for example Shapeless, a type level library in Scala, have artifacts like macro to test compilation error.
Do you have any suggestion for library author to test compiling error?
children: VNode[]; | ||
slots(): any; | ||
data: VNodeData; | ||
parent: Vue; | ||
injections: any | ||
} | ||
|
||
export type PropValidator = PropOptions | Constructor | Constructor[]; |
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.
Do we have any chance to infer Prop
type from Constructor
? Something like the reverse of emitDecoratorMetadata
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 originally tried to do that but the more I thought about it, the more time I realized it'd take to finish. I figured it'd be better to put the PR out and try that at a later stage.
/** | ||
* This type should be used when an array of strings is used for a component's `props` value. | ||
*/ | ||
export type ThisTypedComponentOptionsWithArrayProps<Instance extends Vue, Data, Methods, Computed, PropNames extends string> = |
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.
Can we re-export ThisTyped
option in index? It is useful for library authors, I think.
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.
Is that a great idea? Does re-exporting mean that this is committed to as part of the public interface?
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.
Also, should I just rename this to ComponentOptionsWithArrayProps
?
readonly $slots: { [key: string]: VNode[] }; | ||
readonly $scopedSlots: { [key: string]: ScopedSlot }; | ||
readonly $isServer: boolean; | ||
readonly $data: Record<string, any>; | ||
readonly $props: Record<string, any>; |
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.
We can also make Vue generic so $data
and $props
can be typed. But I wonder if it is an overkill.
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 originally made it generic but ran into problems.
Namely, VueConstructor
has two construct signatures. Each of those signatures has two different sets of type parameters. They each constructed, at least in part, a Vue<Data, Methods, Computed, Props>`, but each gave different type arguments.
When extending a Vue<Data, Methods, Computed, Props>, TypeScript tries to find the right construct signature, but it can't because it's not really possible to make a meaningful set of type arguments for
Vue` that would create the same output type for both signatures.
What it comes down to is that TypeScript has always had an assumption that the type parameters of a construct signature have had a direct correlation with the type parameters of the constructed type - but that's not entirely the case here. At least, that's my understanding of the problem.
So I could try to address this, but maybe down the line. Plus, does anyone need to access $data
and $props
outside of debugging?
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 for the detailed explanation!
Indeed, $data
and $props
are mainly for advanced usage like delegate / wrapper component (example). We would like to check template code in future. So a $props
or $data
might be helpful.
For ordinary users, Record
should be enough.
|
||
static config: { | ||
use<T>(plugin: PluginObject<T> | PluginFunction<T>, options?: T): void; | ||
mixin(mixin: typeof Vue | ComponentOptions<any, any, any, any>): void; |
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.
mixin
does not have ThisTyped
as component
or extend
, is this intended?
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 intended - I don't think ThisType
would help the common scenarios, and I think don't think we can model the scenario all that well. For example, I don't think "get the type of all the methods
from each mixin and intersect them` is possible at the moment.
*/ | ||
export type ThisTypedComponentOptionsWithArrayProps<Instance extends Vue, Data, Methods, Computed, PropNames extends string> = | ||
object & | ||
ComponentOptions<Data | ((this: Record<PropNames, any> & Instance) => Data), Methods, Computed, PropNames[]> & |
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.
👍 for only exposing prop!
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 agree! To be honest this was more a limitation of the type system but it honestly seemed like more of a feature than a bug. 😄
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.
comment it
What's the status with this ? I pulled it locally, and tried it out in my project, and everything seems to work pretty well. Almost negates the need of |
Conflicts: types/vue.d.ts
@HerringtonDarkholme the problem with your suggestion is that it still breaks people who've done module augmentations (because you're moving from an interface to a type alias) but I think that would potentially be okay. It's just something to keep in mind. Would it be okay to pull this in and then revisit that change instead? It might be easier for the core team to gauge the impact of that. |
package.json
Outdated
@@ -118,7 +118,7 @@ | |||
"selenium-server": "^2.53.1", | |||
"serialize-javascript": "^1.3.0", | |||
"shelljs": "^0.7.8", | |||
"typescript": "^2.3.4", | |||
"typescript": "2.5.0-dev.20170615", |
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.
Shouldn't it be depending on a stable version?
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.
It depended on some experimental changes but afaik they landed in 2.4, didn't they @DanielRosenwasser?
@DanielRosenwasser Indeed. After several trials, I think changing to |
@HerringtonDarkholme @DanielRosenwasser Anything that still needs to be done, can i help out with anything? |
@blake-newman I think this pull request needs update dependency and tweak @DanielRosenwasser If you're busy with other things, I'm glad to take this over. |
Hi, @DanielRosenwasser When I clone your branch vue-router,It seems not work. It took me a day to find out why.Fortunately, I found it. This is not sure of the return value。 This is the right to return to rewrite. |
Just pushed a few new commits, but not sure why the build is failing.
|
f1edc6d
to
bb0ff30
Compare
Just amended and re-pushed. Looks like there is a flaky test. |
@HerringtonDarkholme if you can merge and take over, that would be absolutely fantastic. I'd be glad to help however I can. |
Closing with the work being continued in #6391 |
comment it. |
|
This PR improves the Vue
.d.ts
files so that users can get some improved inference when using thenoImplicitThis
compiler option. It takes advantage of many of the features the TypeScript team has been working on over the last few versions. Thus, it requires a minimum TypeScript version of 2.4. You can get this working with the currenttypescript@next
(which is2.5.0-dev.20170614
, but don't let that version number fool you - this will work with the final release of 2.4).The best part about this change is that it makes using Vue easier when users choose not to use classes and inheritance. In other words, users and can more easily
new
up an instance using the Vue constructor, and can create components using theVue.extend
andVue.component
APIs a little more naturally. Extending Vue as a class will still work, so if you prefer to use vue-class-component and the rest, that should still work! However, those dependencies may need to be updated for this change.There's definitely some amount of complexity that's been introduced but I'm willing to iterate on this with the help of others. Given the feedback that we've heard on TypeScript-Vue-Starter, it's been a positive experience for users.
Fixes many of the issues users have asked about in #478.
Tests are probably failing at this point because I need to update the project's dependency on TypeScript.