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 Type declaration file #57

Merged
merged 5 commits into from
Sep 12, 2017
Merged

Conversation

HerringtonDarkholme
Copy link
Member

@HerringtonDarkholme HerringtonDarkholme commented Sep 11, 2017

@yyx990803
Copy link
Member

@regou I don't know if you use TS or not, but feel free to provide feedback.

@HerringtonDarkholme
Copy link
Member Author

also cc @ktsn

@regou
Copy link
Collaborator

regou commented Sep 12, 2017

@yyx990803 Yes, but never used with vue components.
Glad to see so many people love to code their vue project in typescript.
I'll follow this n get started.

And we may absorb some templates as vue-ts-startup or an option of vue-cli?

@kaorun343
Copy link

I've used this plugin with TypeScript, so it's a nice thing that this PR will be merged.

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.

Actually, I'm not familiar with Rx. If my comments are wrong, just let me know 🙂

types/index.d.ts Outdated
declare module "vue/types/vue" {
interface Vue {
$observables: Observables;
$watchAsObservable(exprOrFn: string | Function, options?: WatchOptions): Observable<any>
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 can improve this type by using type parameter like $watch.

$watchAsObservable<T>(fn: (this: this) => T, options?: WatchOptions): Observable<T>

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, returned Observable has a value that is formed of { newValue: T, oldValue: T }?

https://github.com/vuejs/vue-rx#watchasobservableexporfn-options

types/index.d.ts Outdated
interface Vue {
$observables: Observables;
$watchAsObservable(exprOrFn: string | Function, options?: WatchOptions): Observable<any>
$eventToObservable(event: string): Observable<any>
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the returned type is Observable<{ name: string, msg: any }>

types/index.d.ts Outdated
observable: Observable<T>,
next: (t: T) => void,
error: (e: any) => void,
complete: () => void): void
Copy link
Member

Choose a reason for hiding this comment

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

types/index.d.ts Outdated
next: (t: T) => void,
error: (e: any) => void,
complete: () => void): void
$fromDOMEvent(selector: string, event: string): Observable<any>
Copy link
Member

Choose a reason for hiding this comment

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

I guess we can let the return type be Observable<Event> since this function listens DOM event.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, https://github.com/vuejs/vue-rx/blob/master/src/methods/fromDOMEvent.js#L17 selector only applies to native dom element.

declare module 'vue/types/options' {
interface ComponentOptions<V extends Vue> {
subscriptions?: Observables | ((this: V) => Observables)
domStreams?: string[]
Copy link
Member

Choose a reason for hiding this comment

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

We also have observableMethods in component options 😉

https://github.com/vuejs/vue-rx#createobservablemethodmethodname

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.

LGTM

@yyx990803 yyx990803 merged commit ce60b67 into vuejs:master Sep 12, 2017
@yyx990803
Copy link
Member

Thanks @HerringtonDarkholme and reviewers!

@yyx990803
Copy link
Member

Is this typing compatible with post 2.5 types after merging vuejs/vue#6391 ? Do we need an upgrade PR for that?

@ktsn
Copy link
Member

ktsn commented Sep 14, 2017

@yyx990803 Looks like we need to update import Vue = require('vue') to ES Module style import.

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.

5 participants