Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Sep 30, 2019

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This removes the dependency on prepareStackTrace from node internals for #29564

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. repl Issues and PRs related to the REPL subsystem. util Issues and PRs related to the built-in util module. labels Sep 30, 2019
@devsnek
Copy link
Member Author

devsnek commented Sep 30, 2019

cc @nodejs/assert @nodejs/repl @nodejs/util @joyeecheung @bcoe @BridgeAR gosh i hope that's everyone whose stuff was touched

lib/assert.js Outdated Show resolved Hide resolved
Copy link
Member

@BridgeAR BridgeAR left a 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.

const prepareStackTrace = (globalThis, error, trace) => {
// API for node internals to override error stack formatting.
if (internalOverriddenErrors.has(error)) {
const f = internalOverriddenErrors.get(error);
Copy link
Member

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)?

@devsnek devsnek force-pushed the stop-using-preparestacktrace branch from 2cddcf3 to d5b9913 Compare October 2, 2019 03:07
@devsnek devsnek force-pushed the stop-using-preparestacktrace branch from d5b9913 to 93058ba Compare October 2, 2019 03:20
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 2, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Oct 2, 2019

Landed in 70c2444

Trott pushed a commit that referenced this pull request Oct 2, 2019
PR-URL: #29777
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott Trott closed this Oct 2, 2019
@devsnek devsnek deleted the stop-using-preparestacktrace branch October 2, 2019 23:49
BridgeAR pushed a commit that referenced this pull request Oct 9, 2019
PR-URL: #29777
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Oct 10, 2019
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
It relies on nodejs/node#29777, and we don't
override prepareStackTrace.
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
* 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 <shelley.vohr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core. repl Issues and PRs related to the REPL subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants