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

Add typescript definition #3509

Closed
wants to merge 11 commits into from
Closed

Add typescript definition #3509

wants to merge 11 commits into from

Conversation

kaorun343
Copy link
Contributor

@kaorun343 kaorun343 commented Aug 24, 2016

Hi.

This is a definition file for Vue.js 2.0.
I would like to unite the way of providing these definition files, I think, so I took the same way of Vuex.

I referenced a rc.vuejs.org, and it is not complete. These are the TODO List of this.
I hope that this PR will be merged and modified the files to achieve these things by other people.

  • Adding rest definitions since v1.0 such as lifecycle hooks.
  • Adding the functions related to createElement and writing the interface VNode and others.
  • Considering the naming of the interfaces, for instance, whether we should use the same name between TS and Flow.
  • Adding JSDoc/TSDoc comments for IDE. If allowed, I'd like to refer to the official documents.
  • Adding a test if needed as well as we did in DefinitelyTyped
  • Modifying vue-class-component to use this new definition file when this is finished

Thanks.

interface Array<T> {
$remove(item: T): Array<T>;
$set(index: any, val: T): T;
}
Copy link
Member

Choose a reason for hiding this comment

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

Actually, these methods are deprecated on 2.0. We can just remove them.

static compile(template: string): {render: Function, staticRenderFns: Function};
}

declare namespace Vue {
Copy link
Member

Choose a reason for hiding this comment

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

I think we no longer need namespaces for declarations because we can use external module definition format.
Instead of this, I think it would be better to separate files and import the types as needed.

@ktsn
Copy link
Member

ktsn commented Aug 24, 2016

@kaorun343
Thank you for starting write declarations!
I write some comments for this. Please check them 😄

* separate files
* remove unnecessary `{[key: string]: any}`
* from singular to plural
* Add more definitions
* Rename filename and interface
* Sort definitions
* Fix indent
* add test
* fix some definitions
* fix typo
@ericmdantas
Copy link

This is awesome, @kaorun343. I was just thinking the other day how much better Vue'd be with the help of Typescript.

Keep it up! 👍

@kaorun343
Copy link
Contributor Author

I've done my work.


export interface ComputedOptions {
get(this: Vue): any;
set(this: Vue, value: any): void;
Copy link
Member

Choose a reason for hiding this comment

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

ComputedOptions may have cache: boolean property. And all options are optional.

@ktsn
Copy link
Member

ktsn commented Aug 25, 2016

@kaorun343
Thanks! I have written the comments for some critical parts.
LGTM if you fix them 👍

@kaorun343
Copy link
Contributor Author

I've done 👍

@ktsn
Copy link
Member

ktsn commented Aug 25, 2016

Great job 😄

render?(createElement: typeof Vue.prototype.$createElement): VNode;
staticRenderFns?: (() => VNode)[];

init?(): void;

Choose a reason for hiding this comment

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

It seems init is changed to beforeCreated. #2873

@HerringtonDarkholme
Copy link
Member

Great Job! This type definition has very high quality. Kudos to @kaorun343

@yyx990803
Copy link
Member

Is this ready?

@kaorun343
Copy link
Contributor Author

Yes!

@yyx990803
Copy link
Member

Great, can we also add:

  • a npm script that runs the typescript tests
  • include that script in npm test

@yyx990803
Copy link
Member

Manually squashed and merged as dfc64e8 - Thanks for the hard work!

@yyx990803 yyx990803 closed this Aug 31, 2016
@HerringtonDarkholme
Copy link
Member

I have used it locally. There are some issues that block the adoption I think...

  1. no typings field in package.json. Users have to install type definition one more time. This can be easily fixed.
  2. export = Vue in index.d.ts actually does not work for TypeScirpt's module resolution. This is the hard part.

For point1 adding a new field is enough. But point2 needs more elaboration.
Vue exposes Vue as a whole for API consumer. It is good for non-ES6 module users but in ES6 the correct way to import is import * as Vue from "vue". (Babel support this by _interopRequireDefault helper function)

TS cannot export a class as a whole. The workaround hack is export = Vue as submitted in this PR, but the exported symbol must be a namespace or merged with namespace declaration. This is by design, I don't think there is much chance to be changed in TS/ES.

@HerringtonDarkholme
Copy link
Member

HerringtonDarkholme commented Sep 1, 2016

One workaround is changin index.d.ts to this

import {Vue as _V} from './vue'
declare class Vue extends _V {}
declare namespace Vue {}
export = Vue

declare namespace statement is for bypassing export limitation.

declare class Vue extends _V is solely for bypassing import limitation in TS module declaration.

This is hideous hack, and TS users must import * as Vue from "vue", lame. But limits changes in TS specific files.

Another way is adding two private fields to Vue: default pointing to Vue itself and __esModule being true.
This change is compatible with ES5/commonJS/ES6/Babel/TypeScript, and probably future proof. Everyone shall be happy. And TS users can just import Vue from "vue", cool.
But the change needs to be applied to Vue itself.

What's your opinions? @yyx990803 @kaorun343 @ktsn

@ktsn
Copy link
Member

ktsn commented Sep 1, 2016

I think changing index.d.ts is better solution because it can clearly separate the code for adapting Vue's implementation in that file.
If we can insert module.exports.default = Vue and module.exports.__esModule = true on build time, I think it's also good.

@kaorun343 kaorun343 deleted the add-typescript-definition branch September 1, 2016 06:52
@HerringtonDarkholme
Copy link
Member

HerringtonDarkholme commented Sep 1, 2016

@ktsn You mean change something in Babel preset to support module.exports.default = Vue? https://github.com/yyx990803/babel-preset-es2015-rollup-vue

after lengthy web search I think TS users should forget import default and just pay taxes for type checking

@ktsn
Copy link
Member

ktsn commented Sep 1, 2016

@HerringtonDarkholme
Yes, or rollup plugin. But I'm not sure whether such plugin is exist or not.
Probably updating d.ts is realistic. 😉

keyCodes: { [key: string]: number };
}

static extend(options: ComponentOptions): Vue;
Copy link
Member

Choose a reason for hiding this comment

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

It seems extend should return typeof Vue. https://rc.vuejs.org/api/#Vue-extend

@kaorun343 kaorun343 mentioned this pull request Sep 2, 2016
@druppy
Copy link

druppy commented Sep 29, 2016

This is so nice to have d.ts files in the npm packages (for all central components), I really like the way things a going for Vue, I still miss my constructor for classes, to make TS nicer to dance with :-)

But my real issue are, how will a render function look like ? I like to use the new VNode system, from Typescript, but I cant seem to find the prototype anymore (found it in rc7).

Also, has anyone tried to use TSX in any of the modes ?

@HerringtonDarkholme
Copy link
Member

@druppy Thanks for supporting vue. But github issues are mainly for bug report/ feature request. And please file relevant reviews under a pull request.

You can ask question in gitter and forum. And you may also find https://github.com/vuejs/awesome-vue helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants