-
Notifications
You must be signed in to change notification settings - Fork 15.7k
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
express scaffolder should support process.env.PORT #1118
Comments
is this true for heroku + nodejitsu as well? if so im +1 |
also do you guys set NODE_ENV to production? |
heroku yes, and I believe the same for jitsu, but will verify. I'll check on NODE_ENV. |
We don't, but if you think we should, I will file a bug. |
Thinking more about this, I am not sure we should. We allow you to set it, but just because you deploy to the cloud doesn't mean you are going to production. Interested in your thoughts. |
Just checked. Jitsu seems to do some magic that overrides. It doesn't matter what port I choose, though it does work with process.env.PORT as well. Heroku docs say to use process.env.PORT. Cloud9 also uses the same as does Azure. |
yeah some people will be using it for staging etc, I guess best to just leave it. Maybe hint at |
one other issue which is a problem for Azure and Jitsu is this line in the scaffolder. console.log("Express server listening on port %d in %s mode", app.address().port, app.settings.env); For us it is because we are listening on a named pipe, jitsu though generates this error:
|
yeah that was changed a long time ago, it should be moved into the callback now |
aaah, that's a great point. Maybe we should set to production then by default. From an express stand point if you don't, what else does that impact? error messages? |
nvm it's static: https://github.com/visionmedia/express/blob/master/bin/express#L200 |
by default express will |
Would this be a good time to use a setting? Or is it overkill? It needs to be used twice, once to listen, and once to print the port number. I committed my change to my fork to show what I'm talking about. |
yeah it's fine as a setting IMO, I see lots of people doing that anyway |
get port from env in template (fix for #1118)
merged. ps @benatkin you can do "Closes #nnn" and github will automatically close the issue when pushed (might already know but hey) |
Thanks! I knew that it worked for commits to a repo I own, and thought it might for merged commits, but wondered if saying it before it merged was presumptuous. Next time I'll just say it in my commit message! |
(#1118) Thanks Benjamin! Sent from my Windows Phone Reply to this email directly or view it on GitHub: |
@glennblock you're welcome. I enjoyed writing this. I like moving server configuration out of the main repository, to make services portable. Doing it with OAuth client configurations. |
get port from env in template (fix for expressjs#1118)
Currently the scaffolding tool generates code which listens on PORT 3000. This requites modifying the code to listen to process.env.PORT for deploying to cloud hosting providers.
Suggested change would be to use
process.env.PORT || 3000
this would allow the code to work locally or in the cloud without having to make a change.
The text was updated successfully, but these errors were encountered: