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

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

Comments

@OliverJAsh
Copy link

OliverJAsh commented Apr 30, 2024

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)
}
@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

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

@coltonehrman
Copy link

coltonehrman added a commit to coltonehrman/finalhandler that referenced this issue Aug 11, 2024
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
@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
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
Projects
None yet
5 participants