-
Notifications
You must be signed in to change notification settings - Fork 14
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
Simply not working. #33
Comments
Same issue |
I have no idea how I didn't see this when it was first filed. Apologies for that. Can one or both of you post a simple script that can reproduce this? Don't have time at this exact moment to test it. |
@TheAppleFreak finally got it to work: const slackWinston = new SlackHook({
...
emitAxiosErrors: true,
...
}); you will get errors: logger.on('error', function (err) {
console.log(err);
}); |
Ah! That makes sense, then. I kinda live blogged my thoughts as I examined a related issue a while back, but basically what happened was I added what I thought was a bit of error logging logic that turned out to instead lead to unexpected unhandled exceptions when Axios would throw an error for whatever reason. Given as the package had originally been written without any error handling at all and this was (technically) a bug based on past behavior, I added that flag to give consumers the choice whether they wanted to manually handle any exceptions that might get thrown, or silently discard them, defaulting to discarding errors. There's probably a more elegant way of handling this type of stuff than a binary "you handle it yourself" vs "don't do anything" but frankly I'm not sure what a good solution is, given how many users there are of this package. For stuff like 429 errors from Slack, one solution would be to add some sort of rate limiting, but figuring out what type of behavior to follow on that front is difficult. Do you drop incoming messages until the limit is renewed? Queue them up? If you're queuing them, how do you handle the matter of timing urgency? What about other errors, like 403 or 401 errors? Those might indicate that you've put an invalid webhook URL in, but realistically I have no idea where in an app's lifecycle it might be called for the first time (and to my knowledge, Slack doesn't have a mechanism to determine whether a webhook is valid apart from actually sending a message to it). That could very easily lead to crashes in an application while it's handling something really important. I dunno. Might be a good idea to add for a new major version. |
@TheAppleFreak |
The major problem with having it be true by default is that there's a LOT of projects that are expecting the current behavior of "fail silently" (idk how many exactly, but ~20k downloads a week isn't anything to scoff at). Given as the reason the flag even exists at all is because that behavior was causing other people trouble (someone's application was crashing after Slack responded with 429 errors), setting it true by default would basically necessitate a major version bump all on its own to deal with that potentially breaking change. I have my doubts that this is a good idea. |
Installed withouth issue, added slack webhook URL, tested sending an error via Vitest. Test completes, no message sent to Slack.
Using Winston 3.12.0 on Node 20
The text was updated successfully, but these errors were encountered: