mocha
mocha copied to clipboard
mochajs
Append the `cause` stacks to the `Error` stack traces
Requirements
- Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
- All new code requires tests to ensure against regressions.
Description of the Change
Appends the full error stack chain for errors with causes.
Alternate Designs
Prior to the Error cause standardization, there was VError which 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:
- Should this be on by default?
- How should this be formatted?
Why should this be in core?
All current browsers supports this property and Node.js as well since 16.x.
See also eg:
- V8 blog post
- My blog post describing my ponyfill + helpers for it
pinomerging support for it: https://github.com/pinojs/pino-std-serializers/pull/78
Benefits
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
- It can become a bit too verbose if a lot of unnecessary causes gets printed, but is that an issue to take into consideration here?
Applicable issues
- This would be a breaking change for everyone that relies on specific stack output for current error object's containing a stack possessing
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.
This PR hasn't had any recent activity, and I'm labeling it stale. Remove the label or comment or this PR will be closed in 14 days. Thanks for contributing to Mocha!
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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 cause property and the lack of support for them made them difficult to debug.
This PR hasn't had any recent activity, and I'm labeling it stale. Remove the label or comment or this PR will be closed in 14 days. Thanks for contributing to Mocha!
This PR hasn't had any recent activity, and I'm labeling it stale. Remove the label or comment or this PR will be closed in 14 days. Thanks for contributing to Mocha!
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 stale. Remove the label or comment or this PR will be closed in 14 days. Thanks for contributing to Mocha!
Is this going to be merged? we want to start using the cause field in our errors.
coverage: 94.359% (+0.02%) from 94.335% when pulling 571ac7c59ea278df3176e8fde900b9c832881b99 on voxpelli:support-error-causes into b88978deb3c12f9b95502828f6ac29ebe8be85ef on mochajs:master.
Included support for https://github.com/mochajs/mocha/pull/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 ?. is okay yet 😜
😭
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)
