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.

Sign up for GitHub

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

Jump to bottom

Error cause is not displayed #5630

Closed
OliverJAsh opened this issue Apr 30, 2024 · 9 comments · May be fixed by pillarjs/finalhandler#49 or pillarjs/finalhandler#50
Closed

Error cause is not displayed #5630

OliverJAsh opened this issue Apr 30, 2024 · 9 comments · May be fixed by pillarjs/finalhandler#49 or pillarjs/finalhandler#50
Labels
enhancement

Comments

@OliverJAsh
Copy link

OliverJAsh commented Apr 30, 2024 •
edited
Loading

package.json:

{
  "dependencies": {
    "express": "^4.19.2"
  }
}

server.js:

const express = require("express");

const app = express();

app.get("/", (req, res) => {
  throw new Error("foo", {
    cause: new Error("bar"),
  });
});

app.listen(3000, () => {
  console.log("Server is running on port 3000");
});
$ node server.js
Server is running on port 3000
$ curl "localhost:3000"

Actual

The error cause is not displayed.

Error: foo
    at /Users/oliver/Code/reduced-test-cases/express-error-cause/server.js:6:9
    at Layer.handle [as handle_request] (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/layer.js:95:5)
    at next (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/route.js:149:13)
    at Route.dispatch (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/route.js:119:3)
    at Layer.handle [as handle_request] (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/layer.js:95:5)
    at /Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/index.js:284:15
    at Function.process_params (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/index.js:346:12)
    at next (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/index.js:280:10)
    at expressInit (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/middleware/init.js:40:5)
    at Layer.handle [as handle_request] (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/layer.js:95:5)

Expected

The error cause should be displayed, like it is for exceptions occurring outside of request handlers.

Error: foo
    at /Users/oliver/Code/reduced-test-cases/express-error-cause/server.js:6:9
    at Layer.handle [as handle_request] (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/layer.js:95:5)
    ... 7 lines matching cause stack trace ...
    at Layer.handle [as handle_request] (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/layer.js:95:5) {
  [cause]: Error: bar
      at /Users/oliver/Code/reduced-test-cases/express-error-cause/server.js:7:12
      at Layer.handle [as handle_request] (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/layer.js:95:5)
      at next (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/route.js:149:13)
      at Route.dispatch (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/route.js:119:3)
      at Layer.handle [as handle_request] (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/layer.js:95:5)
      at /Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/index.js:284:15
      at Function.process_params (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/index.js:346:12)
      at next (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/index.js:280:10)
      at expressInit (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/middleware/init.js:40:5)
      at Layer.handle [as handle_request] (/Users/oliver/Code/reduced-test-cases/express-error-cause/node_modules/express/lib/router/layer.js:95:5)
}
@OliverJAsh OliverJAsh added the bug label Apr 30, 2024
@wesleytodd
Copy link
Member

Hey @OliverJAsh, thanks for the report. The specification for error causes is comparatively new to the code which handles this and the error stack is what we use for displaying this. I do agree though that showing error causes for development logging would be an awesome improvement. Feel free to open a PR for this one.

@shubham9069
Copy link

Hii @OliverJAsh thanks for the report. can you able to compile this enhancement?

@DanielBelz1997
Copy link

hey, i want to contribute to this enhancement. there is any way to connect this issue to me?

@wesleytodd
Copy link
Member

Hey @DanielBelz1997, we would love a PR for this. The stack trace behavior comes from finalhandler here, that is the best place to start if you would like to work on this.

@DanielBelz1997
Copy link

thanks for the quick reply! i will get right into it

@DanielBelz1997
Copy link

fixed the issue. but the code is in the finalHandler package. how should i do a PR on this?

@wesleytodd
Copy link
Member

wesleytodd commented May 22, 2024 •
edited
Loading

Open a PR to finalhandler and mention this issue in the description of the PR.

@UlisesGascon UlisesGascon mentioned this issue May 27, 2024
Open
@coltonehrman
Copy link

I created a small reproduction/test for this 👀
https://github.com/coltonehrman/bag-of-tools/blob/main/examples/express/error-stack-trace/error.test.ts

coltonehrman added a commit to coltonehrman/finalhandler that referenced this issue Aug 11, 2024
@coltonehrman
feat: handle err.cause & err.stack
25e187a 
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
@coltonehrman coltonehrman mentioned this issue Aug 11, 2024
Open
@wesleytodd
Copy link
Member

I have a review out on both of these PRs. Whoever gets theirs into a fixed up state I will merge and publish. Closing this to track work on this in the respective PRs.

wesleytodd pushed a commit to coltonehrman/finalhandler that referenced this issue Aug 31, 2024
@coltonehrman @wesleytodd
feat: handle err.cause & err.stack
afcdabb 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
5 participants
@OliverJAsh @wesleytodd @coltonehrman @shubham9069 @DanielBelz1997