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

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

Open
andykorth opened this issue Oct 18, 2024 · 5 comments · May be fixed by #29
Open

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

andykorth opened this issue Oct 18, 2024 · 5 comments · May be fixed by #29

Comments

@andykorth
Copy link

andykorth commented Oct 18, 2024

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 [email protected].

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
Copy link

dpopp07 commented Nov 7, 2024

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
Copy link
Author

andykorth commented Nov 7, 2024

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
Copy link

dpopp07 commented Nov 7, 2024

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
Copy link
Author

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 dpopp07 linked a pull request Nov 7, 2024 that will close this issue
@dpopp07
Copy link

dpopp07 commented Nov 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants