-
-
Notifications
You must be signed in to change notification settings - Fork 67
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 definitions #86
Conversation
Still interested in finishing this? :) |
@sindresorhus Sure! I'll pick this up when I have some time 😄 Just a note, I'm not sure my top todo of "Promisifying modules with includes and excludes" is possible in all cases. Do you have any ideas around that? My main concern is that you can't get it fully type safe, so any behaviour here will eventually lead to runtime errors. It may be better to just ignore them on a type level. |
In what cases is it not possible? |
That's fine as long as the cases are documented. |
@tom-sherman Friendly bump :) |
Now I'm revisiting this it looks like excludes/includes are possible to make safe for the most part. I think even exuding the sync methods by default is possible with template literal types. |
@sindresorhus I'm not sure it's possible to support "module functions" here ie. modules with methods. TypeScript doesn't really allow you to work with a type that's callable and also has function properties. We would need to make The workaround would be to Edit: I've changed the semantics to be more clear in tom-sherman@4987750 Hopefully this is ok! |
When excludeMain is false (default) we pify the main function When it's true, we pify the module members. There is unfortunately no way of promisifying both.
Looks good. Thank you :) |
@tom-sherman Are you sure there's no way to handle overloads? Because I already hit this problem in multiple places. For example, https://github.com/sindresorhus/got/runs/8157202709?check_suite_focus=true where the overloads are: export function createCSR(options: CSRCreationOptions, callback: Callback<{ csr: string, clientKey: string }>): void;
export function createCSR(callback: Callback<{ csr: string, clientKey: string }>): void; This limitation makes it kinda annoying to use |
Can you share what the output of The issue we have is that there is no easy way to collect all arguments of an overloaded function as a union of tuples, as far as I'm aware. See microsoft/TypeScript#32164 I'll have another read through this issue though, maybe there's a workaround now. |
Does the workaround I added to the readme help for you? I get that it's not very ergonomic but should work in all cases. |
Yes, that does work: sindresorhus/got@2cd8600 |
Closes #74
Pros and cons compared to #85
Pros:
multiArgs: true
. Although not sure if this is in scope in the context of theerrorFirst
options.Cons:
pify((v: number) => {})()
returnsPromise<unknown>
instead of erroring - at runtime this promise will simply never resolve or reject. A bit of annoying DX but isn't unsafe.Todo:
excludeMain
errorFirst
IssueHunt Summary
Referenced issues
This pull request has been submitted to: