feat: include .cause stacks in the error stack traces
#4829
Conversation
|
This PR hasn't had any recent activity, and I'm labeling it |
|
Still relevant |
|
This PR looks good to me. We'd love for this to land. We had to implement some workarounds in the https://github.com/sequelize/sequelize test suites because our own error messages make heavy use of the |
|
This PR hasn't had any recent activity, and I'm labeling it |
|
Would still love to see this merged |
|
Same here |
|
This PR hasn't had any recent activity, and I'm labeling it |
|
Labeling someone's work as stale because you haven't had time to review is not great :/ |
|
@juergba would be great if you could take a look at this. Node 16 is the oldest still supported version of Node so there are no supported versions anymore that do not have Error.cause |
|
This PR hasn't had any recent activity, and I'm labeling it |
|
Still a lack of review, not this PR itself being stale |
|
Is this going to be merged? we want to start using the |
voxpelli
changed the title
cause stacks to the Error stack traces.cause stacks in the error stack traces
voxpelli
changed the title
.cause stacks in the error stack traces.cause stacks in the error stack traces
|
Included support for #4128 style messages in the cause trail in this update as well, making the stack traces more comparable between top error and cause trail Only thing that's unique to the top error now: Support for pretty chai / assert diffs. |
|
@JoshuaKGoldberg Looks like our linting hasn't understood that |
|
😭 I suppose once we convert to TypeScript & enable the strictest of the typescript-eslint presets, I suppose it'll be caught for us. 😄 (this comment is in jest and does not commit us to any migrations) |
It would be great to get the full error stack chain for errors with causes, especially as all current browsers and Node.js >=16 supports it, see eg https://v8.dev/features/error-cause and https://dev.to/voxpelli/pony-cause-1-0-error-causes-2l2o Eg. `pino` merged support for this as well: pinojs/pino-std-serializers#78
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
voxpelli
force-pushed
the
support-error-causes
branch
from
c12fd74
to
04f7008
Compare
lib/runner.js
Outdated
| const alreadyFiltered = new Set(); | ||
| let currentErr = err; | ||
|
|
||
| while (currentErr?.stack && !alreadyFiltered.has(currentErr)) { |
There was a problem hiding this comment.
?.
From https://github.com/mochajs/mocha/actions/runs/8068123423/job/22040196920?pr=4829:
nps is executing `build` : rollup -c ./rollup.config.js
./browser-entry.js → ./mocha.js...
[!] (plugin at position 4) SyntaxError: Unexpected token (467:22) in /home/runner/work/mocha/mocha/lib/runner.js
lib/runner.js (467:22)
SyntaxError: Unexpected token (467:22) in /home/runner/work/mocha/mocha/lib/runner.js
at Parser.pp$4.raise (/home/runner/work/mocha/mocha/node_modules/rollup-plugin-node-globals/node_modules/acorn/dist/acorn.js:2757:13)
@voxpelli what do you want to do here? My intuition is to delay using ?. until we redo the rollup builds / etc..
There was a problem hiding this comment.
I am now 0 for 2 in making suggestions on this PR. Not great for me. 😂
This reverts commit 04f7008.
JoshuaKGoldberg
added
the
semver-minor
|
Included in v10.4.0 🎉 |
Requirements
Description of the Change
Appends the full error stack chain for errors with causes.
Alternate Designs
Prior to the Error cause standardization, there was
VErrorwhich I did a PR for supporting some years ago #2381. That one as primarily closed because an aversion to supporting random module hooks, which this now being a standard changes.So I think this is now more a matter of:
Why should this be in core?
All current browsers supports this property and Node.js as well since
16.x.See also eg:
pinomerging support for it: Serializeerr.causein a nice way when possible pinojs/pino-std-serializers#78Benefits
Error causes are a great way to enrich errors and to ensure that one can catch all relevant places across an async workflow. See also eg. this old Joyent article explaining how they uses VError.
Possible Drawbacks
Applicable issues
causeproperty, else it would be a minor. So depends on whether the specific stack output is seen as an API that should be stable or whether it's simply a presentation detail.Fixes #2735.