fix(typescript): export EventTypesPayload type

[G-Rath]

This lets us use EventTypesPayload without having to import it the long way, i.e

import { WebhookEvents, Webhooks, sign, verify } from '@octokit/webhooks';
import { EventTypesPayload } from '@octokit/webhooks/dist-types/generated/get-webhook-payload-type-from-event';

I need this type in order to attempt to do narrowing via:

const isSpecificGithubEvent = <TName extends WebhookEvents>(
  event: EventTypesPayload[keyof EventTypesPayload],
  name: TName
): event is EventTypesPayload[TName] => event.name === name;

On an aside: EventTypesPayload isn't entirely correct as it's not the actual event payloads, which makes it harder to write a setup that narrows based on the event name; in general it'd be nice if there was a union type that let you narrow based on the name :/

[gr2m]

On an aside: EventTypesPayload isn't entirely correct as it's not the actual event payloads, which makes it harder to write a setup that narrows based on the event name; in general it'd be nice if there was a union type that let you narrow based on the name :/

you can access the payload with e.g. EventTypesPayload["check_run"]["payload"]. I Agree that EventTypesPayload is maybe not the right name here, maybe just Events might be better, or OctokitEvents to avoid conflicts with other declarations?

We also have * and error keys in there, which is something we will remove in the next version

[G-Rath]

you can access the payload with e.g. EventTypesPayload["check_run"]["payload"]

🤦 I actually didn't think of that - still think it would be nice to export this, but yeah whoops.

I just looked this over again - the problem is that this only works when you know what event you're wanting.

tbh ideally I'd probably recommend swigging the other way: keep the name as is, and drop the WebhookEvent<>, as you can do that at the use-site.

error makes that more complicated - I think you could be able to get the types working, but I'd need to have more of a play around to comment on how much of a change that'd be. I'm happy to help with this if you're interested 🙂

I think the underlying problem is that this library is doing two things: providing the generated types for the events + sign, verify, etc, and exposing an event emitter class for being able to nicely target events + actions. That's not strictly a bad thing, but I don't think it helps the types. (I'm personally using this in a lambda, where the event emitter isn't suitable).

[G-Rath]

@gr2m what about ListenerEvents?

[gr2m]

I'm personally using this in a lambda, where the event emitter isn't suitable

Can we take a step back a second? What's the app you are building? I wonder why you need isSpecificGithubEvent in the first place? I'd like to understand why the Webhooks API is not sufficient for your use case?

@octokit/webhooks and its typescript definitions are up for a bigger refactor, I'd like to learn more about your usecase to inform our process

[G-Rath]

Sure thing, I'm liking where this is going :D

I'm writing an AWS Lambda function for sending an alert to Slack when a user does an action on our org (specifically ones like "make repo public", "add deploy key", etc).

The event emitter pattern works well for standard app servers, as shown in the usage code in the readme, because the server is always on.

This isn't the case for lambdas - somewhat ironically, the "lambda" is like the listener function in the event emitter pattern; the actual "event emitter" in this case is external infrastructure that invokes the lambda when a condition is meet (in this case, it's when Github makes a POST request to a url backed by API Gateway, which is set to invoke my lambda with the http request as it's event).

Effectively, this boils down to the classic "sync vs. async" situation - I could actually use the event emitter, but it'd be painful and over the top; effectively I'd do something like this:

const getGithubEvent = async (
  event: APIGatewayProxyEvent
): Promise<GithubEvent> => {
  const body = JSON.parse(event.body ?? '{}') as Record<string, unknown>;

  return new Promise(async (resolve, reject) => {
    const webhook = new Webhooks();

    webhook.on('*', resolve);
    webhook.on('error', reject);

    await webhook.verifyAndReceive({
      ...body,
      signature: event.headers['X-Hub-Signature']
    });
  });
};

export const handler: APIGatewayProxyHandler = async event => {
  const notifier = new Notifier();

  try {
    const githubEvent = await getGithubEvent();
    const text = describeEvent(githubEvent);

    await notifier.send({
      channel: '#platforms-debug',
      text
    });

    return { body: text, statusCode: 200 };
  } catch (error) {
    await notifier.send({
      channel: '#platforms-debug',
      text: (error as Error).stack
    });

    return { body: 'oh noes!', statusCode: 500 };
  }
};

And that's just to get the event, which is currently got a payload of any - to narrow that, I'll have to do checks against the name; alternatively I could add listeners for the different events, but that'd result in different stringy code, since I'd have to always pass things back up through that promise?

This is vs what I have now:

type WebhookEvents = Exclude<
  keyof EventTypesPayload,
  `${string}.${string}` | 'errors' | '*'
>;

export type GithubEvent<TName extends WebhookEvents = WebhookEvents> = Omit<
  EventTypesPayload[TName],
  'name'
> & { name: TName };

export const isSpecificGithubEvent = <TName extends WebhookEvents>(
  event: GithubEvent,
  name: TName
): event is GithubEvent<TName> => event.name === name;

declare global {
  export namespace NodeJS {
    export interface ProcessEnv {
      // eslint-disable-next-line @typescript-eslint/naming-convention
      GITHUB_WEBHOOK_SECRET: string;
    }
  }
}

export const getGithubEvent = (event: APIGatewayProxyEvent): GithubEvent => {
  const body = JSON.parse(event.body ?? '{}') as Record<string, unknown>;
  const signature = event.headers['X-Hub-Signature'];
  const { GITHUB_WEBHOOK_SECRET } = process.env;

  if (verify(GITHUB_WEBHOOK_SECRET, body, signature)) {
    return body as GithubEvent; // 'verify' is effectively a type-guard
  }

  throw new Error('event did not come from github');
};

const describeEvent = (event: GithubEvent): string => {
  if (isSpecificGithubEvent(event, 'repository')) {
    return `${event.name} was ${event.payload.action} by ${event.payload.sender.login}`;
  }

  return 'hello world';
};

export const handler: APIGatewayProxyHandler = async event => {
  prettyLog('Lambda event', event);

  const notifier = new Notifier();

  try {
    const githubEvent = getGithubEvent(event);
    const text = describeEvent(githubEvent);

    await notifier.send({
      channel: '#platforms-debug',
      text
    });

    return { body: text, statusCode: 200 };
  } catch (error) {
    await notifier.send({
      channel: '#platforms-debug',
      text: (error as Error).stack
    });

    return { body: 'oh noes!', statusCode: 500 };
  }
};

Still have to check the name of the event to narrow for the payload, but that's to be expected and I only have to do it the once.
(My code looks bigger because I've included the whole lambda just for clarity - a lot of the code would be in both examples)

[G-Rath]

I realised I didn't actually answer your question clearly - the reason I need isSpecificGithubEvent specifically is because the current types available don't allow for narrowing.

i.e, this would be ideal:

import { EventPayloads } from '@octokit/webhooks';

interface WebhookEvent<TName extends string, TPayload extends object> {
  id: string;
  name: TName;
  payload: TPayload;
}

type CheckRunEvent = WebhookEvent<'check_run', EventPayloads.WebhookPayloadCheckRun>;
type CodeScanningAlertEvent = WebhookEvent<'code_scanning_alert', EventPayloads.WebhookPayloadCodeScanningAlert>;
type CommitCommentEvent = WebhookEvent<'commit_comment', EventPayloads.WebhookPayloadCommitComment>;

type GithubEvent = CheckRunEvent | CodeScanningAlertEvent | CommitCommentEvent;

const fn = (event: GithubEvent) => {
    if(event.name === 'check_run') {
        // event is type CheckRunEvent 
        console.log(event.payload.organization);
    }

    if(event.name === 'code_scanning_alert') {
        // event is type CodeScanningAlertEvent 
        console.log(event.payload.commit_oid);
    }
}

It would be easy enough to write a script to generate all the types in that shape, but then I'd be maintaining a copy of your types in my code vs its just easier to write a custom typeguard that lets me quickly narrow the type in a way that works well enough as a middle ground.

(having said this, I'm pretty sure you could have the types "build" the current definition of EventPayloads from the above, especially now that we've got string interp in unions, as you could do ${TEvent.name}.${TEvent['payload'].action}, and using templates + Extract/Exclude.... 🤔)

[gr2m]

declare global {
  export namespace NodeJS {
    export interface ProcessEnv {
      // eslint-disable-next-line @typescript-eslint/naming-convention
      GITHUB_WEBHOOK_SECRET: string;
    }
  }
}

Today I learned you can define types for process.env.* variables. Very cool!

in general it'd be nice if there was a union type that let you narrow based on the name :/

I think now I get what you want: A union type of all known GitHub Webhook Events that you can narrow down by checking event.name and potentially narrow down further by checking event.payload.action.

I think that's what we used to have ... I see how that would be valuable now.

I could actually use the event emitter, but it'd be painful and over the top; effectively I'd do something like this:

I'm not as familiar as I wish I would be with AWS. Can you point me to documentation of what event looks like in your handler function?

Given what I understand from your requirement, an idea made up code would looks something like this

export async function handler(event) {
  const notifier = new Notifier();
  const webhooks = new Webhooks(options)

  webhooks.on(['check_run', 'code_scanning_alert', 'commit_comment'], async (event) => {
    await notifier.send({
      channel: "#platforms-debug",
      text: describeEvent(event),
    });
  })

  webhooks.onError(async (error) => {
    await notifier.send({
      channel: "#platforms-debug",
      text: error.stack,
    });
  })

  webhooks.verifyAndReceive(awsEventToOctokitEvent(event))
}

[G-Rath]

I'm not as familiar as I wish I would be with AWS. Can you point me to documentation of what event looks like in your handler function?

Sure thing - I also realised I didn't mention a big important point which is that lambdas have a max execution time, and that their ideal is that once the function returns that's it, you're done with your execution which is why the hop jumping to get the code to be tied to a promise.

(Effectively, when you return a promise in a lambda, it does returnedPromise.finally(finishExecution))

Heres the docs - I'd also recommend checking out the types, which are here.

(There are async lambdas, which I could use but that means I can't return to GH based on if the lambda succeeded or not, and there's more infra setup required for that so its still ideal to support not having an event emitter).

But the event emitter isn't the end of the world, because I can use sign & verify standalone; you've hit the nail on the head with:

A union type of all known GitHub Webhook Events that you can narrow down by checking event.name and potentially narrow down further by checking event.payload.action

That is exactly what'd be good to have - and I'm happy to write/refactor the type generation script to generate such a union; it would even be ok to have both that union and the types are they are currently - its just that there's some thinking to be done on "how do you want your types to look" 😄

[G-Rath]

@gr2m I came up with this, which transforms the types into the unions I need along with strongly typing the action property on payloads that have that:

import {
  EventTypesPayload,
  WebhookEvents
} from '@octokit/webhooks/dist-types/generated/get-webhook-payload-type-from-event';

type WebhookEventName = Exclude<
  WebhookEvents,
  `${string}.${string}` | 'errors' | '*'
>;

type HasPayload<
  TName extends WebhookEventName
> = EventTypesPayload[TName] extends { payload: unknown }
  ? TName //
  : never;

type PayloadAndName<TEventName extends WebhookEventName> = Omit<
  EventTypesPayload[TEventName],
  'id' | 'name'
> & { name: TEventName };

type PayloadsWithNames = {
  [K in WebhookEventName as HasPayload<K>]: PayloadAndName<K>;
};

type ActionsForEvent<
  TName extends WebhookEventName,
  TActions = Extract<WebhookEvents, `${TName}.${string}`>
> = TActions extends `${TName}.${infer TAction}`
  ? TAction //
  : never;

type WithAction<
  TEvent extends { payload: unknown; name: WebhookEventName }
> = TEvent extends { payload: { action: string } }
  ? TEvent & { payload: { action: ActionsForEvent<TEvent['name']> } }
  : TEvent;

type StronglyTypedEvents = {
  [K in keyof PayloadsWithNames]: WithAction<PayloadsWithNames[K]>;
};

export type GithubEventName = keyof StronglyTypedEvents;
export type GithubEvent = StronglyTypedEvents[GithubEventName];

[gr2m]

Heres the docs - I'd also recommend checking out the types, which are here.

Thank you, very helpful! Based on these docs the awsEventToOctokitEvent method in in my dreamcode above would be fairly straight forward, pretty much what we do here:

webhooks.js/src/middleware/middleware.ts

Lines 46 to 49 in fc4ddd3

	const eventName = request.headers["x-github-event"] as WebhookEvents;
	const signatureSHA1 = request.headers["x-hub-signature"] as string;
	const signatureSHA256 = request.headers["x-hub-signature-256"] as string;
	const id = request.headers["x-github-delivery"] as string;

We might event export a method which would let you transform a common request object with headers and body into an Event object as its used by @octokit/webhooks. Or leave it up to userland as its simple enough?

I also realised I didn't mention a big important point which is that lambdas have a max execution time, and that their ideal is that once the function returns that's it, you're done with your execution which is why the hop jumping to get the code to be tied to a promise.

But that shouldn't keep you from using the Webhooks class? It's fairly light weight. I guess the challenge is the code you run in the webhooks.on(['check_run', 'code_scanning_alert', 'commit_comment'], handler) handler callback function. If you expect a long running process, you could use AWS events or whatever it's called? I use arc.codes Events, which is the level of abstraction that makes it usable to me: https://arc.codes/primitives/events

I still think that a union type of all GitHub Webhooks is still usable, but I don't think that you need to go that low level in your case? Am I still missing something?

[gr2m]

const body = JSON.parse(event.body ?? '{}') as Record<string, unknown>;

by the way, if you have the string version of the request body, pass that to the verify method, do not parse it. Because there are cases when that will lead false negatives. We have to turn it back to a string anyway:

webhooks.js/src/sign/index.ts

Lines 35 to 46 in fc4ddd3

	payload =
	typeof payload === "string" ? payload : toNormalizedJsonString(payload);
	return `${algorithm}=${createHmac(algorithm, secret)
	.update(payload)
	.digest("hex")}`;
	}
	function toNormalizedJsonString(payload: object) {
	return JSON.stringify(payload).replace(/[^\\]\\u[\da-f]{4}/g, (s) => {
	return s.substr(0, 3) + s.substr(3).toUpperCase();
	});
	}

[gr2m]

A union type of all known GitHub Webhook Events that you can narrow down by checking event.name and potentially narrow down further by checking event.payload.action

That is exactly what'd be good to have - and I'm happy to write/refactor the type generation script to generate such a union; it would even be ok to have both that union and the types are they are currently - its just that there's some thinking to be done on "how do you want your types to look" 😄

One thing to keep in mind is that we want the defined webhook events to be extendable in future. We are thinking to define a global Octokit namespace, with different interfaces we use across the @octokit/* packages, so that the definitions can be spread across the packages, and both our own packages and users will be able to extend / overwrite them.

See #308

So I guess we should keep the interfaces in order to make them expandable in future, but also use something like your code to transform the interfaces dynamically to a union type, which would include custom events defined by users?

[G-Rath]

that shouldn't keep you from using the Webhooks class?

There's nothing blocking me from using it, but doing so requires awkward hop jumping as well as relies on the assumption that the event will be fired straight away (among other things) - somewhat minor, but in theory there could be infinite time between the two - but this is more just about the complexity of the code that you'd require.

If you expect a long running process, you could use AWS events or whatever it's called?

Sure, except now you've just blow out the size of the infrastructure required from 1 lambda, to n lambdas + an sns topic + a bunch of extra code for passing the events on to the topic, all while still having the same underlying issue of the union + the secondary issue of the event emitter not being a good fit for lambda :)

---

An acceptable solution to this could be to ship the implementation in webhooks.js, so it'd just be a wrapper for what we've posted above in similar forms: a function that returns a promise which resolves/rejects based on the return of an event emitter.

The reason I say this would be fine if done in webhooks.js is that it means you'd be maintaining it, which means it'd not be as prone to potentially subtle bugs if the event emitters behaviour changes (off the top of my head I'm thinking of things like how spawn can throw an error for some specific errors, but otherwise errors will be emitted via the error event).

---

But this is also a secondary problem - even in your dreamcode, the underlying issue still remains: I can't do if (event.name === '<name>') and have it narrow event down.

[gr2m]

doing so requires awkward hop jumping as well as relies on the assumption that the event will be fired straight away (among other things) - somewhat minor, but in theory there could be infinite time between the two - but this is more just about the complexity of the code that you'd require.

I'm not sure what you mean with "there could be infinite time between the two"? The event coming in from Amazon is the webhook event request coming from GitHub, right? Calling webhook.verifyAndReceive will immediately call the event handlers.

I guess you what my dreamcode above is missing is a response? Couldn't you do this?

export async function handler(event) {
  const notifier = new Notifier();
  const webhooks = new Webhooks(options)

  webhooks.on(['check_run', 'code_scanning_alert', 'commit_comment'], async (event) => {
    await notifier.send({
      channel: "#platforms-debug",
      text: describeEvent(event),
    });
  })

  try {
    await webhooks.verifyAndReceive(awsEventToOctokitEvent(event))
    return { statusCode: 200 };
  } catch (error) {
    await notifier.send({
      channel: "#platforms-debug",
      text: error.stack,
    });
    return { body: 'oh noes!', statusCode: 500 }
  }
}

Sorry I think I'm still missing something obvious ...

[gr2m]

But this is also a secondary problem - even in your dreamcode, the underlying issue still remains: I can't do if (event.name === '<name>') and have it narrow event down.

You mean the code wouldn't work in the webhooks.on event handler callbacks? I agree that is something that should be possible

[gr2m]

If you don't like the event handler syntax sugar, would an ideal code look like this?

export async function handler(event) {
  const notifier = new Notifier();
  const webhooks = new webhooks({ secret })

  try {    
    const githubEvent = await webhooks.verifyAndParse(awsEventToOctokitEvent(event))
    const text = describeEvent(githubEvent);
    await notifier.send({
      channel: "#platforms-debug",
      text,
    });

    return { body: text, statusCode: 200 };
  } catch (error) {
    await notifier.send({
      channel: "#platforms-debug",
      text: error.stack,
    });
    return { body: 'oh noes!', statusCode: 500 }
  }
}

[G-Rath]

@gr2m yeah that's exactly what I'd like for this sort of situation.

Don't get me wrong, I love the event emitter pattern - it's just that with lambdas the code snippet you've just provided fits a lot nicer. You definitely can do the same in lambda with the event emitter, it just requires a lot of boilerplate & code overhead.

[gr2m]

thanks for bearing with me, I get it now. This is very, very helpful!

for more complex apps, you might be interested in Probot: which is a framework to create GitHub Apps. I'm currently working on making it play nicely with serverless/function environments, while working well with traditional long-living server processes as well. It's quite challenging to get the APIs right, and your insights already helped me with some questions.

We are currently discussing APIs here: probot/probot#1286 (comment). I'd love to hear your input if you have a moment

[gr2m]

to unblock you for now, should we merge this PR as is?

I'll make a separate issue for webhooks.verifyAndParse and maybe a standalone verifyAndParse export as it seems to be the best solution for your usecase

[G-Rath]

thanks for bearing with me, I get it now.

No problem - looking back over it, I think one of the things I realised I didn't point out is that your code is relying on the implicitness of Node being single-threaded, which is fine but also what makes this pattern more complex for lambda.

to unblock you for now, should we merge this PR as is?

That'd be great, thanks!

[github-actions]

🎉 This PR is included in version 7.18.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀