sindresorhus / globby

User-friendly glob matching
MIT License
2.51k stars 130 forks source link

Add return type for Stats #173

Closed alex-page closed 3 years ago

alex-page commented 3 years ago

This is my first time adding types to an open source package so there might be some mistakes here. I took a go at fixing https://github.com/sindresorhus/globby/issues/139

This should fix up the use case of that should be returning Stats instead of string.

globby('*.tmp', {stats: true});
sindresorhus commented 3 years ago

It needs to return Stats only when stats: true.

alex-page commented 3 years ago

@sindresorhus I am reading some documentation on this now. Is there a recommended approach or example repo you could nudge me in the right direction?

sindresorhus commented 3 years ago

https://github.com/sindresorhus/query-string/blob/main/index.d.ts

alex-page commented 3 years ago

I hit a wall. I have adjusted the test case and just need the string[] return for when {stats: true} does not exist. If anyone can help please jump in!

The globby object containing functions is making this a lot more tricky for me as I can't follow the typescript function overloading mentioned above. I am likely missing something. https://github.com/sindresorhus/globby/blob/c1a3b3244ae992ba7e7e76f501a510ea0d9306df/index.d.ts#L81

Sorry for the extra noise.

sindresorhus commented 3 years ago

Instead of replacing the methods, add overloads.

alex-page commented 3 years ago

I spent a couple hours trying different implementations. I can't figure it out how to add overloads to an already existing method 😔. I tried a few different approaches:

declare function sync(
    patterns: string | readonly string[],
    options?: {stats: true} & globby.GlobbyOptions
): Stats[];

declare function sync(
    patterns: string | readonly string[],
    options?: globby.GlobbyOptions
) : string[] | Stats[];

declare const globby: {
    sync: typeof sync;

I am going to pause work on this PR as this isn't a blocking problem for me.

olup commented 3 years ago

hey @alex-page, what about:

    sync: ((
        patterns: string | readonly string[],
        options?: {stats : true} & globby.GlobbyOptions
    ) => Stats[]) & ((
        patterns: string | readonly string[],
        options?: globby.GlobbyOptions
    ) => string[])
alex-page commented 3 years ago

@olup Getting a similar error

(alias) globbySync(patterns: string | readonly string[], options?: {
    stats: true;
} & globby.GlobbyOptions): Stats[] (+1 overload)
import globbySync
Argument of type 'Stats[]' is not assignable to parameter of type 'string[]'.ts(2345)
olup commented 3 years ago

I am sorry where and how do you get this error ? I just tested it myself and no tests are down. I might propose a PR myself if you are ok with this ? mmh - wait, checking something...

olup commented 3 years ago

You'd better actually omit stats key from the options :

    sync: ((
        patterns: string | readonly string[],
        options: Except<globby.GlobbyOptions, 'stats'> & { stats: true }
    ) => Stats[]) & ((
        patterns: string | readonly string[],
        options?: globby.GlobbyOptions
    ) => string[]);
olup commented 3 years ago

Sorry, to be absolutely fair, this is not the Except bit that does the trick but removing the optional for the option key. This is normal: otherwise, your overload could have options as undefined and then is not distinguishable from the other type. So final code is this :

    sync: ((
        patterns: string | readonly string[],
        options: globby.GlobbyOptions & { stats: true }
    ) => Stats[]) & ((
        patterns: string | readonly string[],
        options?: globby.GlobbyOptions
    ) => string[]);
alex-page commented 3 years ago

This is awesome @olup. I have it working for sync but the Promise<> is a little bit different.

olup commented 3 years ago

You can check my pr, It resolves both cases.