fix(types): accept string event.name for receive()

[jablko]

Since #428 I get the following type error:

webhooks.receive({
  id: request.headers["x-github-delivery"],
  name: request.headers["x-github-event"],
  ^^^^ TS2322: Type 'string' is not assignable to type '"status" | "delete" | "package" | "installation" | "repository" | "team" | "member" | "project" | "public" | "push" | "deployment" | "label" | "milestone" | "release" | "organization" | ... 187 more ... | "workflow_run.requested"'.
  payload: request.body,
});

Instead of changing webhooks.receive() to match eventHandler.receive() (as in #428), can we do the reverse and change eventHandler.receive() to match webhooks.receive() (as in this PR)?

I additionally added | undefined because:

1. It's checked in .receive()

   webhooks.js/src/event-handler/receive.ts

   Lines 46 to 48 in b799113

   	if (!event || !event.name) {
   	throw new AggregateError(["Event name not passed"]);
   	}

2. It avoids having to either non-null assert request.headers["x-github-event"]! or test it before calling .receive() (redundant since it's checked again by .receive()) when running with --noUncheckedIndexedAccess

[wolfy1339]

I disagree with you, I think you should use this code

webhooks.receive({
  id: request.headers["x-github-delivery"],
  name: request.headers["x-github-event"] as EmitterWebhookEventName,
  payload: request.body,
});

[G-Rath]

It's checked in .receive()

That's a runtime check targeted as JS users - allowing undefined defeats the benefit that you get by using TypeScript, since you'll not be told at compile-time if the value you're passing in could be undefined.

[jablko]

@G-Rath Okay, I've removed the | undefined addition from this PR.

@wolfy1339 EmitterWebhookEventName seems like a postcondition of .receive() (it checks event.name against a list of hooks before running the matching handlers) vs. a precondition? What am I missing?

[G-Rath]

What am I missing?

Pretty much the same thing as with the | undefined - it's a runtime check that we do to warn people when they're passing in an event name we don't know about, but if they're using TypeScript we can tell them about this at compile-time rather than as runtime.

Again as with | undefined, your change would mean I'd not know if my variable is valid.

(Also note that we're warning not erroring for this check)

[jablko]

name: string hides the fact that event.name might be bogus from the caller, whereas name: EmitterWebhookEventName hides that fact from the .receive() implementation (if it might be bogus?). Which side is responsible for confirming that event.name is in fact an EmitterWebhookEventName?

The examples in the docs had me thinking that e.g. webhooks.on(["check_run", "code_scanning_alert"], handler) was a convenient way to validate request.headers["x-github-event"].

[G-Rath]

Which side is responsible for confirming that event.name is in fact an EmitterWebhookEventName?

Neither side is - that responsibility lies with the caller of the function (aka whoevers consuming the dependency).

The example in the documentation is using the x-github-event header from a request, which Github sets accordingly and so we expect to be correct. If you're passing in values from other sources, then the responsibility falls on you to ensure they're correct.

We do some basic runtime checking for our JS users for the things that are cheap and easy to check, but we only warn on the actual event names because we don't want to be too restrictive - otherwise the library would be unusable if we were ever missing an event (i.e a bug/typo in our schemas, using a fancy new event that we'd not implemented schemas for, or even maybe custom endpoints or prototyping because you're working at Github on implementing that fancy new event).

[jablko]

The example in the documentation is using the x-github-event header from a request, which Github sets accordingly and so we expect to be correct. If you're passing in values from other sources, then the responsibility falls on you to ensure they're correct.

Isn't webhooks.on(["check_run", "code_scanning_alert"], handler) a convenient way to do that?

[G-Rath]

That would require us to error instead of warn on event names, which we don't want to do because we don't want to be too restrictive.

There are pros and cons to both approaches, but this library has gone the route of warning meaning at the very least changing it to error would be a breaking change.

[G-Rath]

🤦 I used the wrong issue number, sorry - I meant to close #431 with that PR.

[jablko]

I see my mistake: if webhooks.onAny() is going to be handler: (event: EmitterAnyEvent) => any vs. handler: (event: { name: string, payload: unknown }) => any then the input needs to be correspondingly typed. Thanks for your patience and sorry for the noise!