-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUG]: Public interface types should expect strings #1055
Comments
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
This comment has been minimized.
This comment has been minimized.
Here's a set of functions to check if the payload is of proper structure, checks if the event name is a known event. Both functions resolve the types. import { emitterEventNames } from "./generated/webhook-names.js";
import type { EmitterWebhookEventName, EmitterWebhookEvent } from "./types.js";
/**
* Validates if the provided event name or names are a know event name.
*
* @param {string | string[]} event The event name or an array of event names to validate.
* @returns {event is EmitterWebhookEventName | EmitterWebhookEventName[]} True if the event name(s) are valid, otherwise false.
*/
export function validateEventNameOrNames(event: string | string[]): event is EmitterWebhookEventName | EmitterWebhookEventName[] {
if (Array.isArray(event)) {
return event.every(e => emitterEventNames.includes(e as EmitterWebhookEventName));
}
return emitterEventNames.includes(event as EmitterWebhookEventName);
}
/**
* Validates the structure and type of a webhook payload and narrows the type to the corresponding event type.
*
* This function ensures that the provided input matches the expected structure of an event,
* including a valid event name, an ID, and a payload. If the validation passes, the input is
* narrowed to the specific `EmitterWebhookEvent<TName>` type.
*
* @template TName The specific event name to validate against.
* @param {unknown} input The JSON input to validate and type-cast to the event type.
* @returns {input is EmitterWebhookEvent<TName>} True if the input is a valid event payload, otherwise false.
*/
export function validatePayload<TName extends EmitterWebhookEventName>(
input: unknown
): input is EmitterWebhookEvent<TName> {
if (typeof input !== 'object' || input === null || !('name' in input) || !('id' in input) || !('payload' in input)) {
return false;
}
const typedInput = input as { name?: string; id?: string; payload: Record<string, unknown> };
// Check if the event name is valid
const name = typedInput.name;
if (!name || !validateEventNameOrNames(name)) {
return false;
}
if (typeof typedInput.id !== 'string' || typedInput.payload === null || typedInput.payload === undefined) {
return false;
}
return true;
} @jyasskin Are these what you wanted |
What happened?
I tried to call
verifyAndReceive
withname
=request.headers["x-github-event"]
, and got a type error because the header's type isstring
, butverifyAndReceive
expectsWebhookEventName
, which isn't even exported from the library. It turns out thatwebhooks.js/src/middleware/node/middleware.ts
Line 78 in 6531c97
webhooks.js/src/middleware/node/middleware.ts
Line 98 in 6531c97
any
on top of that), which indicates thatverifyAndReceive
should probably just take astring
.Alternatively, the library could expose a function to validate the event name and maybe payload structure, but that seems like more work than just loosening up the types.
A secondary question is whether
receive
should also take looser types forname
andpayload
.webhooks.js/src/verify-and-receive.ts
Lines 41 to 45 in 6531c97
BaseWebhookEvent
to make it correctly infer the payload type when the event name does happen to be constrained.Versions
Typescript 5.6
Relevant log output
No response
Code of Conduct
The text was updated successfully, but these errors were encountered: