-
Notifications
You must be signed in to change notification settings - Fork 79
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
feat(typescript): make verify
a type-guard
#376
Conversation
import { sign } from "../sign/index"; | ||
|
||
type WebhookEvents = Exclude< | ||
keyof EventTypesPayload, | ||
`${string}.${string}` | "errors" | "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woah I didn't know you can do the template literals with string
types! Very cool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thats the nice hotness that landed with 4.1 - it only arrived about 2 weeks ago.
I'm actually very happy with this bit of code, because it's my first sensible reason to use it in production code, and it works like a treat 😍
@@ -10,7 +21,7 @@ export function verify( | |||
secret: string, | |||
eventPayload: object, | |||
signature: string | |||
): boolean { | |||
): eventPayload is GithubEvent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you explain how that helps exactly? I'm not familiar with that notion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem - type guards allow you to define functions that narrow types based on if they return true or false. We actually now also have assertion guards which are the same except the function throws (they landed a few months ago, and very cool).
So by making this a type-guard, you can do this:
const signature = event.headers['X-Hub-Signature'];
const { GITHUB_WEBHOOK_SECRET } = process.env;
const body = JSON.parse(event.body ?? '{}') as object;
if (verify(GITHUB_WEBHOOK_SECRET, body, signature)) {
console.log('Got an event named', body.name, 'from GitHub');
}
In the above, because verify
is a type guard, TypeScript knows that in the scope of the if statement body
has to be of type GithubEvent
when that code is running since it only runs when its true
, and so treats it as such while within the braces of the if statement.
Array.isArray
is an example of a built-in type guard - that function is defined as isArray<T>(arg: T | {}): arg is T[]
.
(it's actually a bit bigger, as it has a conditional check to say "arg is (if T is readonly ? readonly T[] : T[])"
, but I've removed that to keep things simple)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the code in action, vs the current method.
You'll see that the first use of body.name
has an error, whereas the second doesn't:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, I got it now, makes a lot of sense. Thank you for the thorough explanation!
@gr2m I've just confirmed something I suspected but only just gotten far enough to confirm: That makes a lot of what we've been talking about (both on this PR & others) mostly theoretical, since the types I've been trying to consume are not correct for my use-case - ultimately what I'm looking for is types for the payloads, and an easy way to do the verification (I can do it myself, but since this repo provides both types + I'd still be interested in talking about how things could be improved, and contributing to that, but it means this PR isn't right. |
I think this is a good thing, and hopefully just work, but does mean the min TS version required would be 4.1.
I originally posted this in #372 but felt I might as well just PR it for its own discussion.