[Proposal] [TypeScript] prefer interface to conditional types

[shigma]

interface PayloadMap {
  'check_run': WebhookPayloadCheckRun
  'check_run.completed': WebhookPayloadCheckRun
  ...
}

type AllEvents = keyof PayloadMap | 'error' | '*'

export declare type GetWebhookPayloadTypeFromEvent<E = AllEvents, T = WebhookEvent> =
  E extends 'error' ? WebhookEventHandlerError :
  E extends '*' ? any :
  WebhookEvent<PayloadMap[E]> & T

This will not only help reduce dist size but also optimize type check performance.

[gr2m]

Thansk @shigma! I'm curious about "optimize type check performance", could you elaborate?

@dominguezcelada do you have any opinion? Maybe @shigma can start a pull request so we can test the changes and discuss in detail?

[oscard0m]

Thansk @shigma! I'm curious about "optimize type check performance", could you elaborate?

@dominguezcelada do you have any opinion? Maybe @shigma can start a pull request so we can test the changes and discuss in detail?

Let me take a look into it tomorrow.

Also mentioning @ankeetmaini in case you have time to take a look and you want to participate :)

[ankeetmaini]

This looks really cool! I think we can remove the generic parameter T too as it's not used inside the type. But this looks like a much improved way to do this :)

[ankeetmaini]

Is it just me or the ts types are showing any for any event in the project?

Will dig this and see if I can find out why.

[gr2m]

Is it just me or the ts types are showing any for any event in the project?

It's not just you, see #279.

[gr2m]

I had a call with @andrewbranch about types in @octokit/webhooks. Andrew is working on TypeScript at Microsoft.

We talked about how we can define Types in a way that consumers could extend them with custom events, in order to address #277. What we would like to allow is something like this:

declare "@octokit/webhooks" {
  interface Events {
    "my_custom_event": MyCustomEventPayload
  }
}

I think that is very similar to what @shigma suggests, and we would not only simplify our TypeDefinitions and make them more efficient, but also allow for custom types.

@andrewbranch - did I get this right?

@ankeetmaini has a work inprogress PR here: #258

[gr2m]

I gave this some more thought, and I think I'd actually prefer what express is doing by defining a global namespace, see https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/express-serve-static-core/index.d.ts#L17-L25

The reason for that is that it is possible that these type definitions will move to other packages in future, e.g. an @octokit/events package or @octokit/types. My understanding is that this would require changes to the declare "@octokit/webhooks" blocks, while the global namespace approach would always work

declare namespace Octokit {
    export interface Events {
        "my_custom_event": MyCustomEventPayload
    }
}

[github-actions]

🎉 This issue has been resolved in version 7.12.1 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[oscard0m]

I gave this some more thought, and I think I'd actually prefer what express is doing by defining a global namespace, see https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/express-serve-static-core/index.d.ts#L17-L25

The reason for that is that it is possible that these type definitions will move to other packages in future, e.g. an @octokit/events package or @octokit/types. My understanding is that this would require changes to the declare "@octokit/webhooks" blocks, while the global namespace approach would always work

declare namespace Octokit {
    export interface Events {
        "my_custom_event": MyCustomEventPayload
    }
}

Is this still a thing to cover @gr2m @ankeetmaini ? Would it solve #277 ? (just catching up with PR's, is not clear to me if we have this sorted out 😇 )

[ankeetmaini]

In Typescript interfaces merging is allowed. I'll have to test how it'll pick up as the interfaces would be in different files. We might need to wrap the interface in a module.

[oscard0m]

In Typescript interfaces merging is allowed. I'll have to test how it'll pick up as the interfaces would be in different files. We might need to wrap the interface in a module.

Maybe we can try a dummy project or draft an example on Typescript Playground? From here I would be happy to draft a PR or help (I'm not that into TS yet)

[gr2m]

Is this still a thing to cover @gr2m @ankeetmaini

Yes.

I want to give it more thought, a global Octokit namespace would affect all Octokit libraries. We can start experimenting now, I just don't want to rush ahead and build it, to avoid disruptive changes once we put it all together with the types from the REST API endpoints, etc

[oscard0m]

Is this still a thing to cover @gr2m @ankeetmaini

Yes.

I want to give it more thought, a global Octokit namespace would affect all Octokit libraries. We can start experimenting now, I just don't want to rush ahead and build it, to avoid disruptive changes once we put it all together with the types from the REST API endpoints, etc

Sounds good to me. Do you want me to create a new issue for this so we keep track on it? :)

[gr2m]

that'd be great thank you!