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 definitions #86

Merged
merged 16 commits into from Sep 2, 2022
Merged

Conversation

tom-sherman
Copy link
Contributor

@tom-sherman tom-sherman commented Dec 18, 2021

Closes #74

Pros and cons compared to #85

Pros:

  • No need for as many overloads
  • Supports as many generics as you like
  • Support for as many callback arguments as you like when multiArgs: true. Although not sure if this is in scope in the context of the errorFirst options.

Cons:

  • pify((v: number) => {})() returns Promise<unknown> instead of erroring - at runtime this promise will simply never resolve or reject. A bit of annoying DX but isn't unsafe.

Todo:


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


@sindresorhus
Copy link
Owner

Still interested in finishing this? :)

@sindresorhus sindresorhus changed the title Add typescript definitions Add TypeScript definitions Jan 23, 2022
@tom-sherman
Copy link
Contributor Author

@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.

@sindresorhus
Copy link
Owner

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?

In what cases is it not possible?

@sindresorhus
Copy link
Owner

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.

That's fine as long as the cases are documented.

@sindresorhus
Copy link
Owner

@tom-sherman Friendly bump :)

More obvious tests

Generic af

Saner tests

WIP

WIP
@tom-sherman
Copy link
Contributor Author

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.

@tom-sherman tom-sherman marked this pull request as ready for review August 31, 2022 12:25
@tom-sherman
Copy link
Contributor Author

tom-sherman commented Aug 31, 2022

@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 excludeMain produce never as the return type. What do you think?

The workaround would be to pify each method and the main function separately.

Edit:

I've changed the semantics to be more clear in tom-sherman@4987750

Hopefully this is ok!

index.test-d.ts Outdated Show resolved Hide resolved
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.
@sindresorhus sindresorhus merged commit e13efc7 into sindresorhus:main Sep 2, 2022
@sindresorhus
Copy link
Owner

Looks good. Thank you :)

@sindresorhus
Copy link
Owner

@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 pify and the old types (@types/pify) could handle it.

@tom-sherman
Copy link
Contributor Author

Can you share what the output of pify(createCSR) would be usinng @types/pify? As I understand it you'd just get Promise<any>? If so then @types/pify handles it by not handling it at all, there is very little type safety here.

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.

@tom-sherman
Copy link
Contributor Author

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.

https://github.com/sindresorhus/pify#why-is-pify-choosing-the-last-function-overload-when-using-it-with-typescript

@sindresorhus
Copy link
Owner

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

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

Successfully merging this pull request may close these issues.

Add TypeScript type definition
2 participants