Skip to content

Added typings #2

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added typings #2

wants to merge 1 commit into from

Conversation

eXeDK
Copy link

@eXeDK eXeDK commented May 9, 2017

Added some typings for this lovely SDK.

Copy link
Member

@tjconcept tjconcept left a comment

Choose a reason for hiding this comment

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

Very nice! And a better introduction for me to Type definitions than reading any tutorials 😉

My main concern is whether I can keep this up-to-date when doing update in the future. It's a (minor) downside that the types are not in the code itself, it makes them prone to falling behind.

I will have another look one of the days and rigorously go through all sources to see if you have missed some methods etc. and then merge it.

}
export interface PaylikeErrorConstructor extends Error {
new (message: string): PaylikeError
(message: string): PaylikeError
Copy link
Member

Choose a reason for hiding this comment

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

These two lines, are they to say that you can instantiate both with and without the new keyword? Or?

Copy link
Author

Choose a reason for hiding this comment

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

I would assume so. I'm not 100% sure, this is from https://github.com/Microsoft/TypeScript/blob/master/lib/lib.es6.d.ts

before: (id: string) => Cursor<T>
limit: (limit: number) => Cursor<T>
stream: (highWaterMark: number) => any
toArray: () => Promise<Array<T>>
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the missing methods as well? https://github.com/paylike/node-api#cursors-pagination

Copy link
Author

Choose a reason for hiding this comment

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

Yes they and easily be added. I'll leave that as an exercise for you ;)
Unfortunately there are typings for pull-stream, that's why stream returns any right now.

@eXeDK
Copy link
Author

eXeDK commented May 9, 2017

Well keeping the definitions up to date is an issue when not writing Typescript yourself, then it would be a product of you publishing your NPM package.
So it would be something that you would have to update, or your users would have to make PR to update it from time to time.

@stemaDev
Copy link

Very nice! And a better introduction for me to Type definitions than reading any tutorials 😉

My main concern is whether I can keep this up-to-date when doing update in the future. It's a (minor) downside that the types are not in the code itself, it makes them prone to falling behind.

I will have another look one of the days and rigorously go through all sources to see if you have missed some methods etc. and then merge it.

well it seems that your concern is not valid as it's been 6 years without any commits in the repo. I think this should be merged so that people could use the official package when using typescript and not have to implement their own or to hack this .d.ts into their project

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

Successfully merging this pull request may close these issues.

3 participants