-
Notifications
You must be signed in to change notification settings - Fork 68
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
support node v18 #349
support node v18 #349
Conversation
🦋 Changeset detectedLatest commit: 5801c0c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@lukasIO let me know if this looks good. |
.github/workflows/ci.yml
Outdated
@@ -70,14 +70,17 @@ jobs: | |||
|
|||
test: | |||
name: Test | |||
strategy: | |||
matrix: | |||
node-version: [18, 20] |
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.
That's good thinking, let's add 22 as well while we're at it.
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.
added 22
and latest
for future proofing
return globalThis.crypto.subtle.digest(algorithm, data); | ||
} else { | ||
const nodeCrypto = await import('node:crypto'); | ||
const hash = nodeCrypto.createHash(algorithm.replace('-', '').toLowerCase()); |
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.
I don't believe we're likely to change the algorithm here in the foreseeable future. I'd rather go with hard coded algorithm names here and above instead of the string manipulation.
@@ -67,7 +68,7 @@ export class WebhookReceiver { | |||
const claims = await this.verifier.verify(authHeader); | |||
// confirm sha | |||
const encoder = new TextEncoder(); | |||
const hash = await crypto.subtle.digest('SHA-256', encoder.encode(body)); | |||
const hash = await digest('SHA-256', encoder.encode(body)); |
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.
instead of converting this back and forth, we can pass the body directly and only do the textencoding part in the global webcrypto path
@lukasIO ready for another review! |
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.
this looks great, thank you!
Extract out the digest method to support other implementations. Use this as a fallback if the web crypto api is not available. As a result we're now able to support node versions < 19. Node 18 is still officially supported via maintenance related updates, and so setting the minimum supported version to node v18.