-
Notifications
You must be signed in to change notification settings - Fork 30k
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
lib: stop using prepareStackTrace #29777
Conversation
cc @nodejs/assert @nodejs/repl @nodejs/util @joyeecheung @bcoe @BridgeAR gosh i hope that's everyone whose stuff was touched |
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.
It would be great to add a custom eslint rule to error on all prepareStackTrace()
usages. Otherwise it's probably difficult to prevent new ones sneaking in. The warning could then reference internalOverridenErrors
.
lib/internal/errors.js
Outdated
const prepareStackTrace = (globalThis, error, trace) => { | ||
// API for node internals to override error stack formatting. | ||
if (internalOverriddenErrors.has(error)) { | ||
const f = internalOverriddenErrors.get(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.
Nit: maybe use at least fn
to indicate it's a function (that's how we mostly named them throughout the code)?
2cddcf3
to
d5b9913
Compare
d5b9913
to
93058ba
Compare
Landed in 70c2444 |
PR-URL: #29777 Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #29777 Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
It relies on nodejs/node#29777, and we don't override prepareStackTrace.
* chore: bump node in DEPS to v12.16.0 * Fixup asar support setup patch nodejs/node#30862 * Fixup InternalCallbackScope patch nodejs/node#30236 * Fixup GN buildfiles patch nodejs/node#30755 * Fixup low-level hooks patch nodejs/node#30466 * Fixup globals require patch nodejs/node#31643 * Fixup process stream patch nodejs/node#30862 * Fixup js2c modification patch nodejs/node#30755 * Fixup internal fs override patch nodejs/node#30610 * Fixup context-aware warn patch nodejs/node#30336 * Fixup Node.js with ltcg config nodejs/node#29388 * Fixup oaepLabel patch nodejs/node#30917 * Remove redundant ESM test patch nodejs/node#30997 * Remove redundant cli flag patch nodejs/node#30466 * Update filenames.json * Remove macro generation in GN build files nodejs/node#30755 * Fix some compilation errors upstream * Add uvwasi to deps nodejs/node#30258 * Fix BoringSSL incompatibilities * Fixup linked module patch nodejs/node#30274 * Add missing sources to GN uv build libuv/libuv#2347 * Patch some uvwasi incompatibilities * chore: bump Node.js to v12.6.1 * Remove mark_arraybuffer_as_untransferable.patch nodejs/node#30549 * Fix uvwasi build failure on win * Fixup --perf-prof cli option error * Fixup early cjs module loading * fix: initialize diagnostics properly nodejs/node#30025 * Disable new esm syntax specs nodejs/node#30219 * Fixup v8 weakref hook spec nodejs/node#29874 * Fix async context timer issue * Disable monkey-patch-main spec It relies on nodejs/node#29777, and we don't override prepareStackTrace. * Disable new tls specs nodejs/node#23188 We don't support much of TLS owing to schisms between BoringSSL and OpenSSL. Co-authored-by: Shelley Vohr <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesThis removes the dependency on
prepareStackTrace
from node internals for #29564