fix(types): add explicit return type to webhooks() function

[oscard0m]

Fix #200

[oscard0m]

@G-Rath tried some combinations with Generics and I'm not able to make the tests pass. In all of them the octokit expected prop fails. Probably I'm missing something with TS.

Feel free to modify this PR and merge when working. I have to go to sleep now.

[oscard0m]

Maybe @wolfy1339 has some TS magic to share here? 😄

[wolfy1339]

Maybe @wolfy1339 has some TS magic to share here? smile

Sorry. I haven't really looked into this.

[gr2m]

Thanks @oscard0m!

I think this fails because the return value is not Webhooks, but Webhooks with a custom TTransformed

[gr2m]

I was hoping that we won't need to set the Type parameters anymore with v8 😕

[oscard0m]

Nice. I was trying to make it compile in my local but I was missing this second generic argument part.

---

I would like to find a way to let TypeScript auto-infer the type yes... @G-Rath do you think it would be possible (in a different PR) to get this BaseWebhookEvent inference properly solved?

[G-Rath]

The solution would be to export BaseWebhookEvent, which in itself might not be the worst idea but for this specific case is: the resulting type will be every possible element of the union, which is huge.

Instead, providing the type explicitly is the better solution, as it's for a public export - relying on inference means it'd be possible for the types to subtly change without knowing (even via say a package update if we were to change the types upstream).

[gr2m]

relying on inference means it'd be possible for the types to subtly change without knowing (even via say a package update if we were to change the types upstream).

that would also include fixes though, right?

providing the type explicitly is the better solution

as it is right now in this PR? Or would you change anything?

[G-Rath]

that would also include fixes though, right?

Yes, but actually you'll never get fixes, as I just realised this'll never actually be inferred: type definitions have to be explicitly typed in full, as they don't have the source code which is required for TypeScript to infer the code.

i.e

export const fn = () => 'hello world!';

will always give you:

export const fn: () => string;

---

So for any "fixes" we'd always have to publish a new version for the type definitions to include them.
If we rely on inference, we won't know that the types need to be updated, since it won't cause an error.

[G-Rath]

as it is right now in this PR? Or would you change anything?

It looks good to me as is. I suspect it'll need changing when octokit/webhooks.js#464 is merged

[github-actions]

🎉 This PR is included in version 10.1.2 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[gr2m]

It looks like with the latest changes of @octokit/webhooks we no longer need to set the return type explicitly, and I don't think there are any performance regressions from the type inference @G-Rath

The typescript definitions build with @pika/pack for the published package look like this

// pkg/dist-types/webhooks.d.ts
import { Octokit } from "@octokit/core";
import { Webhooks } from "@octokit/webhooks";
import { Options } from "./types";
export declare function webhooks(appOctokit: Octokit, options: Required<Options>["webhooks"]): Webhooks<{
    octokit: Octokit;
    id: string;
    name: "push";
    payload: import("@octokit/webhooks-definitions/schema").PushEvent;
} | {
    octokit: Octokit;
    id: string;
    name: "public";
    payload: import("@octokit/webhooks-definitions/schema").PublicEvent;
} | {
// ... many more events here
} | {
    octokit: Octokit;
    id: string;
    name: "workflow_run";
    payload: import("@octokit/webhooks-definitions/schema").WorkflowRunEvent;
}>;

Compared to the current

import { Octokit } from "@octokit/core";
import { Webhooks, EmitterWebhookEvent } from "@octokit/webhooks";
import { Options } from "./types";
export declare function webhooks(appOctokit: Octokit, options: Required<Options>["webhooks"]): Webhooks<EmitterWebhookEvent & {
    octokit: Octokit;
}>;

I guess in the end it's the same thing? Or do you think that the big union will be a significant performance impact, and I just don't see it on my machine?

[G-Rath]

do you think that the big union will be a significant performance impact, and I just don't see it on my machine?

I don't see it as a massive performance problem, but it'll likely make the types harder to work with - I'd expect error messages to be something like "type does not match x, y, ... 20 others, z" instead of "type does not match Webhooks", and wouldn't be surprised if it caused some loss of quality for IDEs as they tend to provide different autocompletion and coloring based on if a type is a union or not (plus they have to resolve the properties in the sum of the union).

It also bloats the package out unnecessarily, and like I said earlier if the types change in a way that would be breaking, we'd not actually know meaning there's a higher risk of disrupting downstream consumers.

On the flip side it also means if we change our transformation we won't be told if the type output changes.

These are all edge cases in their own right, but imo its not worth saving us a couple of characters in our source code given the likely hood it'd need changing.

[gr2m]

These are all edge cases in their own right, but imo its not worth saving us a couple of characters in our source code given the likely hood it'd need changing.

Agreed, thank you for the clarifications. I'll add a reference to this PR for future reference on why we added an explicit return type in #204