-
-
Notifications
You must be signed in to change notification settings - Fork 362
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 #20
Conversation
Thanks for submitting this. Can you follow this style guide: https://github.com/sindresorhus/typescript-definition-style-guide |
hey, I have changed typings according to review |
Thanks. Still needs tests and some more things mentioned in the style guide. Try going through it point for point. |
index.d.ts
Outdated
put (input: Request | string, options?: Options): Promise<Response>; | ||
patch (input: Request | string, options?: Options): Promise<Response>; | ||
head (input: Request | string, options?: Options): Promise<Response>; | ||
delete (input: Request | string, options?: Options): Promise<Response>; |
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.
There's a lot of duplication on these. Is there any way to reuse the declaration?
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 far as I know, there is nothing in typescript can reuse these declarations. at least axios declare in this way
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.
Sometimes, in simple cases, copy-paste doesn't hurt and is even simpler. I think this is such case when it's totally fine. Easily digestable by readers
Also note this:
I don't see that reflected in the type definition. |
Hey, types test has been added, and some changes. Hope it will work fine |
index.d.ts
Outdated
|
||
/** | ||
* Create a new ky instance with some defaults overridden with your own. | ||
* @returns new Ky instance |
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.
new Ky instance
=> New Ky instance.
// @brandon93s @calebboyd In case you would be able to help review :) |
Nice work, @luv-sic. Looks good now :) |
Fixes #12