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

Create TypeScript typings #5

Open
mstuart opened this issue Oct 2, 2020 · 14 comments
Open

Create TypeScript typings #5

mstuart opened this issue Oct 2, 2020 · 14 comments

Comments

@mstuart
Copy link
Contributor

mstuart commented Oct 2, 2020

This project isn't written in TypeScript, but many TypeScript users would like to use published typings. For this issue, create TypeScript typings that cover the exported functions, and contribute it back to DefinitelyTyped!

This issue is great for someone who wants to learn more about TypeScript!

@ghost
Copy link

ghost commented Oct 4, 2020

assign me.

@mnicpt mnicpt assigned ghost Oct 4, 2020
@mstuart
Copy link
Contributor Author

mstuart commented Oct 5, 2020

Hi @jinkya, all yours!

@mstuart
Copy link
Contributor Author

mstuart commented Oct 21, 2020

Hi @jinkya, just wanted to follow up. Can you still take this up? If not, wanted to leave it open for others before the end of the month. Thanks!

@daniel-shuy
Copy link

I'd like to take this up

@AnshMishra2001
Copy link

Can I work on this with @daniel-shuy

@daniel-shuy
Copy link

Sorry @AnshMishra2001, I'm almost done, just left with the test cases. I'll link the PR here once its ready if you want to review it.

@daniel-shuy
Copy link

daniel-shuy commented Oct 3, 2021

@bluepnume I have a question:

static try<X, Y, C : mixed, A : $ReadOnlyArray<mixed>>(method : (...args : $ReadOnlyArray<mixed>) => (ZalgoPromise<X> | Y), context? : C, args? : A) : ZalgoPromise<X | Y> {

Should the type for method.args be A instead of $ReadOnlyArray<mixed>? I noticed that this was changed in be355ad#diff-3c3f49644853b3ef49c7bcd9dd361ae0b8262bb7869f2bf3e369b2068a711756L407

I'm wondering if I should define it in TypeScript as:

static try<X, C, A extends readonly any[]>(method: (...args: readonly any[]) => ZalgoPromise<X>, context?: C, args?: A): ZalgoPromise<X>;
static try<Y, C, A extends readonly any[]>(method: (...args: readonly any[]) => Y, context?: C, args?: A): ZalgoPromise<Y>;

or:

static try<X, C, A extends readonly any[]>(method: (...args: A) => ZalgoPromise<X>, context?: C, args?: A): ZalgoPromise<X>;
static try<Y, C, A extends readonly any[]>(method: (...args: A) => Y, context?: C, args?: A): ZalgoPromise<Y>;

Or is this done intentionally so that it can be called with partial arguments? eg.

ZalgoPromise.try((...args: [boolean, number, string]) => {
    console.error(args);
}, this, [false]);  // pass less arguments than defined parameters

if so, then I can define it as:

static try<X, C, A extends readonly any[]>(method: (...args: A) => ZalgoPromise<X>, context?: C, args?: Partial<A>): ZalgoPromise<X>;
static try<Y, C, A extends readonly any[]>(method: (...args: A) => Y, context?: C, args?: Partial<A>): ZalgoPromise<Y>;

@daniel-shuy
Copy link

Also, another question:

Do we need to support mixed ZalgoPromise and raw types? eg.

ZalgoPromise.resolve(0)
    .catch(error => {
        if (error) {
            return `${error}`;
        } else {
            return ZalgoPromise.resolve(false);
        }
    });

Currently catch() (and other transformation functions) are defined as:

catch<X, Y>(onError : (error : mixed) => ZalgoPromise<X> | Y) : ZalgoPromise<X | Y> {

The above works in Flow because type parameters default to void, but doesn't work in TypeScript because type parameters default to unknown.

This means that:

catch<X, Y>(onError: (error: any) => ZalgoPromise<X> | Y): ZalgoPromise<X | Y>;

in TypeScript, when called without explicitly specifying type parameters, eg.

ZalgoPromise.resolve(0)
    .catch(error => `${error}`);

would infer X and Y as unknown and string, which will return a ZalgoPromise<unknown> (anything union with unknown becomes unknown).

To avoid having to explicitly specifying type parameters, function overloading can be used instead of union types, eg.

catch<X>(onError: (error: any) => ZalgoPromise<X>): ZalgoPromise<X>;
catch<Y>(onError: (error: any) => Y): ZalgoPromise<Y>;

This however means that the onError callback must return either ZalgoPromise<X> or Y, not both.

@daniel-shuy
Copy link

Created a PR at DefinitelyTyped/DefinitelyTyped#56179

@mstuart mstuart assigned daniel-shuy and unassigned ghost Oct 4, 2021
@mstuart
Copy link
Contributor Author

mstuart commented Oct 4, 2021

Thanks @daniel-shuy!

@daniel-shuy
Copy link

An issue that I discovered while working on this is that the Quick Start section in the README is wrong/outdated for UMD and CommonJS. It should be:

<!-- UMD -->
<script src="zalgo-promise.js"></script>
<script>
    new ZalgoPromise.ZalgoPromise( ... );
</script>
// CommonJS
var ZalgoPromise = require('zalgo-promise');

new ZalgoPromise.ZalgoPromise( ... );

See https://stackblitz.com/edit/typescript-zdsko6?file=index.ts

@daniel-shuy
Copy link

daniel-shuy commented Oct 5, 2021

@mstuart @bluepnume should I hold off merging the PR for you to review once its approved?

@daniel-shuy
Copy link

@mstuart @bluepnume the PR has been approved, should I go ahead and merge it?

@daniel-shuy
Copy link

Since there's no reply I've gone ahead and merged it. The type definitions are now released in npm (@types/zalgo-promise)

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

No branches or pull requests

3 participants