-
Notifications
You must be signed in to change notification settings - Fork 42
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
fix(Webhook): unknown error in catch block #73
Conversation
✔️ Deploy Preview for topgg-node-sdk-docs ready! 🔨 Explore the source changes: 79bb5a7 🔍 Inspect the deploy log: https://app.netlify.com/sites/topgg-node-sdk-docs/deploys/619b895c6689420008f41f14 😎 Browse the preview: https://deploy-preview-73--topgg-node-sdk-docs.netlify.app |
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.
Sorry for the long wait for the review 😞
@@ -126,7 +126,7 @@ export class Webhook { | |||
res.sendStatus(204); | |||
} | |||
} catch (err) { | |||
this.options.error?.(err); |
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'm confused, what exactly is getting thrown from functions that isn't an error? If we're handling all errors with a 500 response, shouldn't they also be treated as an error on the library side? Maybe the correct move here should be to change the type of the error callback from Error
to unknown
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.
Are we sure we want to do this? It would be a breaking change. I'm not opposed but a major bump seems unnecessary.
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.
Well, this is also a breaking change technically, just more subtle
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.
Alright let's just get this in and remember to do the type change for next major release
No description provided.