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

Improve Vue type declarations for more canonical usage #5887

Closed
wants to merge 22 commits into from

Conversation

DanielRosenwasser
Copy link

@DanielRosenwasser DanielRosenwasser commented Jun 14, 2017

This PR improves the Vue .d.ts files so that users can get some improved inference when using the noImplicitThis 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 current typescript@next (which is 2.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 the Vue.extend and Vue.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.

@yyx990803
Copy link
Member

yyx990803 commented Jun 15, 2017

Thanks @DanielRosenwasser ! Are there any remaining incompatibilities aside from the need for using latest dependancies, e.g. required change in tsconfig?

Also this would cover #5016 as far as I can tell.

@HerringtonDarkholme
Copy link
Member

hmmm, the test is failing.

It seems Base constructor return type 'CombinedVueInstance<Data, Methods, Computed, Record<PropNames, any>>' is not a class or interface type..

So interface augmentation fails.

propsData?: Object;
computed?: { [key: string]: ((this: V) => any) | ComputedOptions<V> };
methods?: { [key: string]: (this: V, ...args: any[]) => any };
Copy link
Member

@HerringtonDarkholme HerringtonDarkholme Jun 15, 2017

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?

Copy link
Author

@DanielRosenwasser DanielRosenwasser Jun 15, 2017

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!

Copy link
Member

@HerringtonDarkholme HerringtonDarkholme Jun 15, 2017

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.

@DanielRosenwasser
Copy link
Author

@yyx990803 the fact that ComponentOptions used to be generic on 1 type arg, and now has 4 unrelated type arguments, will cause some incompatibilities. That's the main one you'll run into with vue-class-component.

@DanielRosenwasser
Copy link
Author

The build should now be fixed.

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.

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

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 😂

Copy link
Author

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

Copy link
Member

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

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 newed 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....

Copy link
Author

@DanielRosenwasser DanielRosenwasser Jun 15, 2017

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.

@DanielRosenwasser
Copy link
Author

I just sent out a fix. In the previous iteration, extend inherited members on the static side instead of the instance side. I've now done the opposite. Give it another go - I've added the test that @ktsn mentioned above.

@DanielRosenwasser
Copy link
Author

DanielRosenwasser commented Jun 15, 2017

@yyx990803 I think I could undo the commit if you don't want to fix #5016 right away.

Copy link
Member

@HerringtonDarkholme HerringtonDarkholme left a 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[];

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

Copy link
Author

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> =

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.

Copy link
Author

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?

Copy link
Author

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>;

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.

Copy link
Author

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?

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;

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?

Copy link
Author

@DanielRosenwasser DanielRosenwasser Jun 16, 2017

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[]> &

Choose a reason for hiding this comment

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

👍 for only exposing prop!

Copy link
Author

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. 😄

Copy link

Choose a reason for hiding this comment

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

comment it

@championswimmer
Copy link

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 vue-class-component

@DanielRosenwasser
Copy link
Author

DanielRosenwasser commented Aug 1, 2017

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

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?

Copy link
Member

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?

@HerringtonDarkholme
Copy link
Member

@DanielRosenwasser Indeed.

After several trials, I think changing to interface ComponentOptions<V, D=any, M=any, C=any, P=any> might introduce fewer upgrading issues.

@blake-newman
Copy link
Member

blake-newman commented Aug 15, 2017

@HerringtonDarkholme @DanielRosenwasser Anything that still needs to be done, can i help out with anything?

@HerringtonDarkholme
Copy link
Member

@blake-newman I think this pull request needs update dependency and tweak Props option type.

@DanielRosenwasser If you're busy with other things, I'm glad to take this over.

@AlexStacker
Copy link

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。
typescript_2
typescript_3

This is the right to return to rewrite.
typescript_1
typescript
I just started to learn Vue recently, do not know to rewrite the right.
That branch, I can't open an issue, and that's relevant, so I submitted it here.

@DanielRosenwasser
Copy link
Author

DanielRosenwasser commented Aug 16, 2017

Just pushed a few new commits, but not sure why the build is failing.

SUMMARY:
✔ 969 tests completed
✖ 1 test failed

FAILED TESTS:
  Transition mode
    ✖ transition out-in on async component (resolve after leave complete)
      PhantomJS 2.1.1 (Linux 0.0.0)
    Expected 1 to be 0.
    webpack:///test/unit/features/transition/transition-mode.spec.js:480:44 <- index.js:149624:44
    shift@webpack:///test/helpers/wait-for-update.js:24:32 <- index.js:90813:36

@DanielRosenwasser
Copy link
Author

Just amended and re-pushed. Looks like there is a flaky test.

@DanielRosenwasser
Copy link
Author

@HerringtonDarkholme if you can merge and take over, that would be absolutely fantastic. I'd be glad to help however I can.

@yyx990803
Copy link
Member

Closing with the work being continued in #6391

@pkf1994
Copy link

pkf1994 commented Aug 9, 2020

comment it.

@chenyayong
Copy link

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.

None yet