fix: Error cause is not displayed
#49
Conversation
UlisesGascon
changed the title
cause is not displayed
|
I enabled the CI runners :) |
index.js
Outdated
| @@ -175,6 +180,11 @@ function getErrorMessage (err, status, env) { | |||
| // use err.stack, which typically includes err.message | |||
| msg = err.stack | |||
|
|
|||
| // if error cause included | |||
| if (err.cause) { | |||
| msg += `\n[cause]: ${err.cause.stack}` | |||
There was a problem hiding this comment.
The template literal strings are the cause of the CI failures. These are not supported in the older versions of node. Ideally we can just swap these for plain strings and then we can update them when we drop older node versions with the next major.
There was a problem hiding this comment.
hey! fixed it. i think that should work.
There was a problem hiding this comment.
Thanks! I am gearing up for some travel, but I hope to loop back on this asap. Sorry things take so long sometimes, this is an entirely volunteer group so life and work usually come first. I promise we are working on it though.
There was a problem hiding this comment.
Thanks for your awesome contribution @DanielBelz1997 🥳
While I was reading your PR, I feel I need your help to understand the change. I would also like to request you to revert formatting changes.
| var defer = typeof setImmediate === 'function' | ||
| ? setImmediate | ||
| : function (fn) { process.nextTick(fn.bind.apply(fn, arguments)) } | ||
| var defer = |
There was a problem hiding this comment.
Can you please revert the formatting change?
| @@ -41,20 +44,22 @@ var isFinished = onFinished.isFinished | |||
| */ | |||
|
|
|||
| function createHtmlDocument (message) { | |||
| var body = escapeHtml(message) | |||
There was a problem hiding this comment.
Requesting help to understand, do we really need change this? Perhaps it is already handling multiple line error message?
There was a problem hiding this comment.
Also just to chime in, this kind of thing is exactly why (as requested in @IamLizu's other comment) we try to avoid unnecessary formatting changes. It makes it harder to figure out what is a functional change needing attention and what is purely presentation and can be ignored.
| @@ -175,6 +180,13 @@ function getErrorMessage (err, status, env) { | |||
| // use err.stack, which typically includes err.message | |||
| msg = err.stack | |||
|
|
|||
| // if error cause included | |||
There was a problem hiding this comment.
Does it handle nested cause? What if the cause itself has another cause? Also, perhaps it should append the stack trace of the error itself, regardless of whether the cause exists or not?
| @@ -253,9 +265,7 @@ function getResponseStatusCode (res) { | |||
| */ | |||
|
|
|||
| function headersSent (res) { | |||
| return typeof res.headersSent !== 'boolean' | |||
There was a problem hiding this comment.
Please revert it, let's try to make less unwanted changes so its easier for the folks to review
|
|
||
| request(server) | ||
| .get('/') | ||
| .expect(500, /[Error: bar]/, done) |
There was a problem hiding this comment.
May want to modify this test case a bit as it does not confirm the cause trace is printed anywhere.
I wrote a similar test case here I wrote a similar test case here if you want to use it https://github.com/coltonehrman/bag-of-tools/blob/main/examples/express/error-stack-trace/error.test.ts if you want to use some parts of it.
The test case I wrote confirms the existing functionality of it (ie: without the cause trace), and would need to be modified to include the cause trace somewhere.
There was a problem hiding this comment.
I think you addressed this in your new PR, and since we haven't heard back from @DanielBelz1997 on reverting the formatting changes, I am considering closing this in favor of #50. @DanielBelz1997 opened theirs first, I want to give them a chance to fix this PR. I have some feedback I will leave on #50 as well, and if those are addressed first we will move forward with #50 instead of this one.
This is a replacement for pillarjs#49. I just pulled out the specific change related to this issue and added it here. Closes: expressjs/express#5630
This is a replacement for pillarjs#49. I just pulled out the specific change related to this issue and added it here. Closes: expressjs/express#5630 test: Error w/ cause case fix: recursively find Error.cause stacks Remove the error.stack logic as it is redundant test: check for both 1 level & 2 level Error.cause refactor: use native util.format() API for Error printing fix: put back original lines of code test: update tests to be less brittle
the 'cause' value in the Error class were not included in the response text.
This PR closes expressjs/express#5630