feat: TypeScript support for Collection

[MunifTanjim]

No description provided.

[gr2m]

Oh that looks great, thank you for putting that together Munif! I'll test it with my @octokit libraries, which are all written with TypeScript and I use the collection API

[gr2m]

Very sorry Munif, I totally dropped the ball on this one. I'm looking into it now

[gr2m]

Instead of using O, R, E, could you use something more clear, such as TOptions, TResult, TError?

Otherwise this is great, really appreciate the thorough docs updates!

[MunifTanjim]

Instead of using O, R, E, could you use something more clear, such as TOptions, TResult, TError?

Just to clarify, you want to change the shape of the generic that Hook.Collection accepts, right?

So, instead of:

type HooksType = {
  add: { O: { type: string }; R: { id: number }; E: Error };
  save: { O: { type: string }; R: { id: number } };
  read: { O: { id: number; foo: number } };
  destroy: { O: { id: number; foo: string } };
};

const hooks = Hook.Collection<HooksType>();

You want this:

type HooksType = {
  add: { Options: { type: string }; Result: { id: number }; Error: Error };
  save: { Options: { type: string }; Result: { id: number } };
  read: { Options: { id: number; foo: number } };
  destroy: { Options: { id: number; foo: string } };
};

const hooks = Hook.Collection<HooksType>();

In that case, I think this going to be very verbose, as I need to write this repetative thing. That's why I went with the short names. What do you think about that?

---

Or did you mean to just change the internal names in index.d.ts files like these

type BeforeHook<O> = (options: O) => void
type ErrorHook<O, E> = (error: E, options: O) => void

to

type BeforeHook<Options> = (options: Options) => void
type ErrorHook<Options, Error> = (error: Error, options: Options) => void

?

[gr2m]

I think verbosity is better, it makes the code easier to read, and we read code much more often then we write it. It makes the code easier to maintain and contribute to. I try to avoid abbreviations as a general rule, so I would use more verbose type variable names for both, internal usage and the public APIs

[MunifTanjim]

I've changed those to Options, Result and Error and updated the README to reflect that.

I've also kept the option to use O, R, E in case anybody prefers a shorter variant. But let me know if you don't want to keep that option, I'll remove them.

[gr2m]

Thanks a lot! I'll test your changes with @octokit/core

[gr2m]

It seems to be working while working in TypeScript, but currently my build step with tsc fails when trying to update @octokit/core.

The error I'm getting is

➜  core.js git:(master) ✗ /Users/gregor/Projects/octokit/core.js/node_modules/typescript/bin/tsc --outDir /Users/gregor/Projects/octokit/core.js/pkg/dist-src/ -d --declarationDir /Users/gregor/Projects/octokit/core.js/pkg/dist-types/ --project /Users/gregor/Projects/octokit/core.js/tsconfig.json --target es2020 --module esnext --noEmit false --sourceMap false
src/index.ts:117:5 - error TS2322: Type 'HookCollection<Record<string, { O: any; R: any; E: any; }>, string>' is not assignable to type 'HookCollection<HookType, "request">'.
  Property 'request' is missing in type 'Record<string, { O: any; R: any; E: any; }>' but required in type 'HookType'.

any idea what that could be?

[MunifTanjim]

You probably forgot to pass the type on line 73. It should be:

    const hook = new Collection<HookType>()

otherwise TypeScript won't let you assign it to the property type on line 178

  hook: HookCollection<HookType>

at line 117

    this.hook = hook;

P.S: You can omit the second generic type from HookCollection<HookType, "request"> (if you want). TypeScript will automatically derive it from HookType.

[gr2m]

You probably forgot to pass the type on line 73. It should be:

    const hook = new Collection<HookType>()

That was exactly it. Thank you so much!

[gr2m]

One last question. I think this change is breaking for TypeScript users of the HookCollection type. It seems that a type parameter is now required.

src/index.ts(178,9): error TS2707: Generic type 'HookCollection<HooksType, HookName>' requires between 1 and 2 type arguments

Is there a simple way to workaround that? No worries if not, I don't consider type-only changes breaking changes, but if it's simple enough, I think it would reduce some friction when people upgrade

[MunifTanjim]

Is there a simple way to workaround that?

Sure. Making it required was not intentional. Fixed it.

[gr2m]

Sorry @MunifTanjim, I've upgraded @octokit/core without locally without making any code changes, but am now getting a different build error:

/Users/gregor/Projects/octokit/core.js/node_modules/typescript/bin/tsc --outDir /Users/gregor/Projects/octokit/core.js/pkg/dist-src/ -d --declarationDir /Users/gregor/Projects/octokit/core.js/pkg/dist-types/ --project /Users/gregor/Projects/octokit/core.js/tsconfig.json --target es2020 --module esnext --noEmit false --sourceMap false
src/index.ts:117:5 - error TS2322: Type 'HookCollection<Record<string, { O: any; R: any; E: any; }>, string>' is not assignable to type 'HookCollection<Record<string, { Options: any; Result: any; Error: any; }>, string>'.
  Type 'Record<string, { O: any; R: any; E: any; }>' is not assignable to type 'Record<string, { Options: any; Result: any; Error: any; }>'.
    Index signatures are incompatible.
      Type '{ O: any; R: any; E: any; }' is not assignable to type '{ Options: any; Result: any; Error: any; }'.

117     this.hook = hook;
        ~~~~~~~~~

Sounds like an incompatibility with the type parameter names?

[MunifTanjim]

Sorry, was my bad. Missed a spot while making the type args optional.

[gr2m]

That was it, seems all to be working great now!

[gr2m]

Thank you so much, this is really great!

[gr2m]

🎉 This PR is included in version 2.2.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀