-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(node-core): Extend onnhandledrejection with ignore errors option #17736
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
Conversation
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
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 delay but LGTM overall! I had some suggestions to reuse some helpers but feel free to merge this once addressed!
const isObject = reason !== null && typeof reason === 'object'; | ||
if (!isObject) { |
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.
l: we can reuse isPlainObject
here
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.
plain object will return false for isPlainObject(new Error('')))
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.
ah right, ok then let's leave it as is, thanks for checking!
return { name: '', message: String(reason ?? ''), isObject }; | ||
} | ||
|
||
const errorLike = reason as Record<string, 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.
using isPlainObject
above should also let us get rid of the type cast here
const nameMatches = | ||
matcher.name === undefined || | ||
(typeof matcher.name === 'string' ? errorInfo.name === matcher.name : matcher.name.test(errorInfo.name)); | ||
|
||
const messageMatches = | ||
matcher.message === undefined || | ||
(typeof matcher.message === 'string' | ||
? errorInfo.message.includes(matcher.message) | ||
: matcher.message.test(errorInfo.message)); | ||
|
||
return nameMatches && messageMatches; |
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'd recommend reusing isStringOrRegex
and isMatchingPattern
here, to streamline this a bit :)
// name/message matcher | ||
const nameMatches = matcher.name === undefined || isMatchingPattern(errorInfo.name, matcher.name, true); | ||
|
||
const messageMatches = matcher.message === undefined || isMatchingPattern(errorInfo.message, matcher.message); |
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.
Bug: Inconsistent Pattern Matching Causes Unhandled Rejections
The isMatchingPattern
function is called inconsistently for error names and messages. Name matching includes a third true
parameter, while message matching omits it. This leads to different matching behaviors (e.g., case sensitivity) and potentially unexpected ignore
functionality for unhandled rejections.
I’m introducing support for selectively suppressing specific errors. This is done by adding a new option that allows users to override the default logging modes. By passing this option down through the configuration, users will have finer control over which errors are logged versus ignored. Also added a default array for the errors, addressing an issue where Vercel’s flush is called during abort, causing unnecessary error logs.