-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
process: add reportError #41912
base: main
Are you sure you want to change the base?
process: add reportError #41912
Conversation
Review requested:
|
@@ -268,6 +268,7 @@ let knownGlobals = [ | |||
setInterval, | |||
setTimeout, | |||
queueMicrotask, | |||
reportError, |
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 should probably go under this comment, with the same caveat
Lines 296 to 301 in 6ec2253
// TODO(@ethan-arrowood): Similar to previous checks, this can be temporary | |
// until v16.x is EOL. Once all supported versions have structuredClone we | |
// can add this to the list above instead. | |
if (global.structuredClone) { | |
knownGlobals.push(global.structuredClone); | |
} |
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.
@aduh95 I don't understand why - this PR will land on the next semver-major - other versions won't have this in common/index.js so they won't expect the global and will (rightfully) error if it's present right?
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.
IIRC that’s there so make test-doc
can pass without needing to compile the latest master
.
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 be +1 on this landing in Node.js as a global. Node.js core it self uses similar utility APIs already, here's a few occurrences I could find:
node/lib/internal/event_target.js
Lines 915 to 917 in 43371dd
function emitUncaughtException(err) { | |
process.nextTick(() => { throw err; }); | |
} |
node/lib/internal/debugger/inspect.js
Line 177 in c12db60
}), (error) => process.nextTick(() => { throw error; })); |
node/lib/internal/process/warning.js
Lines 145 to 151 in 09c9e5d
if (process.throwDeprecation) { | |
// Delay throwing the error to guarantee that all former warnings were | |
// properly logged. | |
return process.nextTick(() => { | |
throw warning; | |
}); | |
} |
node/lib/internal/webstreams/adapters.js
Lines 290 to 295 in 29f714b
// In a next tick because this is happening within | |
// a promise context, and if there are any errors | |
// thrown we don't want those to cause an unhandled | |
// rejection. Let's just escape the promise and | |
// handle it separately. | |
process.nextTick(() => { throw error; }); |
* `error` {Error|any} an error to report. | ||
|
||
The `reportError(error)` method triggers an uncaught exception with the error | ||
`error`. This will crash the Node.js process by default. |
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.
might be good to link to uncaught exception handling somewhere here so people can take action on the "will crash" bit of the doc.
46b3224
to
9e4ce6f
Compare
6c54878
to
7e42d77
Compare
// The user called reportError with a non-error so we want to give them | ||
// a way to get a stack trace. We do this by emitting a warning the user | ||
// so the user can run node with `--trace-warnings` to get a stack tace. | ||
process.emitWarning('reportError was called with a non-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.
hmm.... I'm not convinced the warning is necessary or all that useful. Not blocking however.
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 mostly thing we shouldn’t encourage users to error with things without a stack trace
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.
Does the browser warn on this?
Not every error condition needs a stack trace.
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.
Not every error condition needs a stack trace.
I am pretty sure when debugging a warning I would really want a stack trace though note this is opt in with --trace-warnings
right?
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.
The individual user’s desire there may be for a stack trace, but that’s not universal, or the language wouldn't let you throw something without one.
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.
@ljharb as far as I'm aware the language is not yet aware of stack traces anyway? So the language is completely oblivious to the debugging experience (or the user) isn't it?
The only reason stuff like Error
even exists is that language itself can throw stuff that subclasses it (like RangeError or TypeError) and as a possible extensibility point. The language is specific about this:
Instances of Error objects are thrown as exceptions when runtime errors occur. The Error objects may also serve
as base objects for user-defined exception classes.
(exists since es3)
In any case aren't "what errors users get" and "what the language lets you do" here completely orthogonal since it's something the runtime and not the language typically does?
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 - users (should) expect to be able to throw and catch any kind of value, because every kind of value can be an error. Thus, a warning here on an otherwise-valid error will be surprising and confusing to developers who expect that explicitly passing a non-Error object to reportError will be calmly accepted.
This needs a rebase (Node.js 18.x semver cutoff is next week). |
I am honestly still kind of not sure we should actually do this - waiting for more people to ask for this or to weigh in this is a good idea |
I would fricken love this |
Fixes: #38947
Spec: https://html.spec.whatwg.org/#runtime-script-errors
I'm opening as draft for discussion - mostly I want feedback on "are we sure crash on next tick" is the right behavior :] (I think it is but want opinions).
This is missing the wpts but that probably shouldn't block landing.
cc @mcollina @addaleax @Linkgoron