expressjs / errorhandler

Development-only error handler middleware
MIT License
427 stars 66 forks source link

Environment check in README documentation is out of date / not working? #28

Open andykorth opened 3 weeks ago

andykorth commented 3 weeks ago

Description

The README documentation for errorhandler (under "Simple example") suggests registering the errorhandler like this:

  // only use in development
  app.use(errorhandler())
}

However, when I launch my express.js app with npm run dev it does not actually execute that line. Other places I look suggest checking your dev environment like this: if ( app.get('env') === 'development' ) { ...

This does work for me. Which is correct? Are there more addendums to the examples than I realize?

I'm using express@4.21.0.

The documentation in the README is also mirrored to https://expressjs.com/en/resources/middleware/errorhandler.html

Expectations

README documentation should work, as-is.

dpopp07 commented 6 days ago

The command npm run dev implies a custom dev script in your package.json. That's where any development-environment details would be set up. So, how you check the environment depends on what your particular project does in that script.

What is your npm run dev script doing?

Edit: Note that the default environment value is development. So, if you never set the NODE_ENV value, the environment is assumed to be "development". That may be why if ( app.get('env') === 'development' ) works but if (process.env.NODE_ENV === 'development') doesn't. In the first, you're checking the app's assumed environment. In the other, you're looking directly at the environment variable (and if you aren't setting it yourself, it won't be set). Either are recommended but the example in the README assumes you are being explicit about setting the NODE_ENV environment variable.

andykorth commented 6 days ago

Hi dpopp07, thanks for the reply.

My package.json is looks like:

{
  "name": "my-app",
  "version": "1.0.0",
  "main": "index.js",
  "scripts": {
    "build": "npx tsc",
    "start": "node dist/server.js",
    "dev": "nodemon src/server.ts",
    "lint": "eslint",
    "test": "jest",
....
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
....
    "cors": "^2.8.5",
    "dotenv": "^16.4.5",
    "errorhandler": "^1.5.1",
    "express": "^4.21.0",
    "mongodb": "^6.9.0",
    "ts-node": "^10.9.2"
  },

I believed it was all basically the default stuff, although another team member made it.

Also I note a line was missing in my initial post, the Simple Example for expressjs/errorhandler suggests:

if (process.env.NODE_ENV === 'development') {
  // only use in development
  app.use(errorhandler())
}

I was missing the top line, the most important one. 🤦

Thank you!

dpopp07 commented 6 days ago

Ah - there is your issue! If you change

"dev": "nodemon src/server.ts",

to

"dev": "NODE_ENV=development nodemon src/server.ts",

the example will work. The NODE_ENV variable is something you need to set explicitly - it is not set for you. (In addition, you may want to add NODE_ENV=production to your "start" script to follow best practices).

andykorth commented 6 days ago

Thank you very much for your help! I'm feeling like this doesn't need to be added to the errorhandler documentation, because it's something everyone knows, or it's something that is covered elsewhere, then. Is that your feeling too? Thanks again!

dpopp07 commented 6 days ago

You're very welcome!

I'm feeling like this doesn't need to be added to the errorhandler documentation, because it's something everyone knows, or it's something that is covered elsewhere, then.

I don't disagree - I think it's something of a standard for express - but the fact that you opened the issue makes me think it could be more clear 🙂 So I opened #29. Let me know if you don't think that would've cleared things up for you.