Should NODE_ENV be set by the buildpack?

[zeke]

As @aseemk notes on the Heroku Node forum:

This might not be new to the diet buildpack, but I just noticed that the Node docs no longer say you guys set NODE_ENV=production. Is that right? If so, may I ask why the change?

Dev Center has two articles that reference NODE_ENV=production, one about Express caching and the other a sample .profile.d. script.

Dare I ask, should NODE_ENV be set automatically by the buildpack? PATH is currently the only environment variable it touches.

cc @ddollar @dpiddy @jclem @mojodna @ryanbrainard @rwdaigle @friism

[mojodna]

👎

My inclination is that it shouldn't. Being opinionated by defaulting it (but allowing it to be overridden) might be one thing, but even there it feels a little intrusive. It's also specific to particular Node packages rather than being Node-wide.

[aseemk]

I thought that it used to be set by Heroku in the past, so I was more curious why the change. But I might be wrong about it having been set; I don't see a trace of this on archive.org.

I don't feel strongly whether it should be set or not. FWIW, though, I don't think it's very specific to Express; it seems to have become a common convention across modules.

(E.g. https://github.com/jeresig/i18n-node-2/blob/master/i18n.js#L19 that I noticed recently)

[danp]

On the ruby side, the ruby buildpack adds this bit to .profile.d/ruby.sh for rack/rails apps to default these things to production:

export RACK_ENV=${RACK_ENV:-production}
export RAILS_ENV=${RAILS_ENV:-production}

These can be overridden by explicit config on the app.

At least with the convention around these environment variables in the ruby world I think it makes sense to default them to production on the platform as they generally disable things you would only want locally.

What would be bad about defaulting NODE_ENV to production in a similar way? I don't know enough about its use in the node world.

[naaman]

👍 on putting something like this in .profile.d/node.sh

[naaman]

Note: see '.profile.d scripts' doc for background.

[kennethreitz]

I feel like, generally, having multiple environment targets is against one of the core principals of 12factor.

Though, making a sane default in a ~/.profile.d/node.sh is a great idea if the community/language has a primitive for this already. In bundler, environment targets are encouraged, so having a sane default makes sense there. In Python, we have no such pattern.

[rwdaigle]

I'm with @dpiddy – think it should be set by default and easily overridden. I think it's our responsibility to provide a sane default configuration for apps running on our platform. Having prod=true feels the sanest of all options (not set, set to dev, etc...).

[friism]

+1 for buildpacks setting sane production-y defaults that make sense when running on Heroku.

[mojodna]

Ok, consider me persuaded, provided that it can be overrided (which it sounds like we're talking about).

[zeke]

Thanks for the input everyone. I'm gonna add this to .profile.d/node.sh:

export NODE_ENV=${NODE_ENV:-production}

[NathanJang]

Is this still true? #77's discussions says it was rolled back.

[hunterloftis]

@NathanJang no longer true; this is the current default env:

heroku-buildpack-nodejs/lib/environment.sh

Lines 1 to 5 in aeac9d6

	create_default_env() {
	export NPM_CONFIG_PRODUCTION=${NPM_CONFIG_PRODUCTION:-true}
	export NPM_CONFIG_LOGLEVEL=${NPM_CONFIG_LOGLEVEL:-error}
	export NODE_MODULES_CACHE=${NODE_MODULES_CACHE:-true}
	}

However, keep in mind that when setting NPM_CONFIG_PRODUCTION=true, npm will set NODE_ENV=production for the subshells it spawns for things like postinstall scripts.

[aseemk]

That's disappointing to hear. Could the sane NODE_ENV=production default be brought back, but in the right way this time so that it does get overridden by heroku config:set NODE_ENV=...?

Thanks guys!

[aseemk]

(I'm not able to re-open this issue.)

[hunterloftis]

@aseemk users use heroku apps for all sorts of deployments (production, staging, automatic github branch testing). During the build, we set NPM_CONFIG_PRODUCTION=true by default to minimize build times, but other than that I'd like to leave environment settings to the user - especially given the volume of tickets we get from users confused by the env we currently set.

[NathanJang]

@hunterloftis: in my situation I need NPM_CONFIG_PRODUCTION=false because I need devDependencies to be installed, but I also need a simple way to check if the app is currently deployed in production (on Heroku) rather than in development.

[NathanJang]

I was confused because I was expecting it to be set for me based on #77 and this issue. Could the title/top post be edited so it's clearer?

[hunterloftis]

@NathanJang, you can just heroku config:set NODE_ENV=production. For example:

$ NODE_ENV=production NPM_CONFIG_PRODUCTION=false npm install

> [email protected] install /Users/hloftis/test/express/node_modules/time
> node-gyp rebuild

  CXX(target) Release/obj.target/time/src/time.o
  SOLINK_MODULE(target) Release/time.node
  SOLINK_MODULE(target) Release/time.node: Finished

> [email protected] postinstall /Users/hloftis/test/express
> echo NODE_ENV=$NODE_ENV

NODE_ENV=production

[NathanJang]

@hunterloftis Yes I know; I'm just saying I was confused by being led to this method by StackOverflow, finding this issue, but not seeing it set for me. It just wasn't clear from the comments that it's no longer the case. Thanks for all your help! 😂

[aseemk]

@hunterloftis: I hear you. And I sympathize that this may be a breaking change (if customers rely on NODE_ENV not being set). I'll leave it in your hands, but I just want to toss out one parting thought:

Consider that not setting NODE_ENV by default is having an opinion: that by default, Node apps on Heroku should not cache Express views, not cache Browserify/Webpack bundles, not cache i18n strings, etc.

(Those aren't trivial things btw. In our app, we timed uncached Express view rendering at 50ms (vs 1-2ms cached). And our template engine used to do file reads synchronously (that has since been fixed), which means that Node was locked for those 50ms!)

Heroku certainly does have an opinion on best practices by default. H12 timeouts are a perfect example: arguably unnecessary for Node (given this rationale), but it's a best practice. Same with 12 Factor.

So... food for thought. Thanks, and good luck. =)

[aseemk]

Funny coincidence: 10 mins after posting this, I was browsing Twitter and saw this tweet/link:

The Effects of Omitting NODE_ENV in Your Express.js Apps: http://apmblog.dynatrace.com/2015/07/22/the-drastic-effects-of-omitting-node_env-in-your-express-js-applications/

https://twitter.com/JavaScriptDaily/status/624217744574128128

[hunterloftis]

@aseemk thanks for the link. I generally share the opinion of 12factor.net:

In a twelve-factor app, env vars are granular controls, each fully orthogonal to other env vars. They are never grouped together as “environments”, but instead are independently managed for each deploy. This is a model that scales up smoothly as the app naturally expands into more deploys over its lifetime.

A better practice is to explicitly set view cache on express instead of implicitly using NODE_ENV. Similarly, for i18n, you should explicitly set devMode.

That said, I just did a quick spot check for NODE_ENV on the last 20 node builds. Six had NODE_ENV (or some other config, eg VIEW_CACHE) set while fourteen left it undefined. So, it does look like we're doing a disservice to non-expert users who aren't aware of their configuration options. Setting NODE_ENV=production by default will give such users a speed boost, while experts will be using explicit mechanisms anyway and will be minimally affected.

The problem with .profile.d scripts is, if you use them for output, they're super chatty (as evidenced by the WEB_CONCURRENCY stuff that I'm going to mute). However, if you don't show what .profile.d is doing, monkeying with the env can lead to confusion and frustration. I'll work up a PR and we can find a good mechanism. @aseemk @NathanJang, you volunteering to be beta testers? ;)

[aseemk]

Just to clarify my opinion, I'm not at all suggesting to use NODE_ENV to name different environments. We set NODE_ENV to production on every single one of our Heroku apps: production, staging, development, testing. We do not use NODE_ENV to identify environments — we use granular configs, as recommended by 12 Factor (and I agree). The reality is simply that NODE_ENV is looked at, and things are optimized when it's set to production, by many different modules. So I see it as "must set to production everywhere except local development, as a Node.js best practice".

(You're right that we could be explicit about those optimizations. Alas, we take advantage of the NODE_ENV=production convenience.)

Happy to help beta test as needed. Good luck. =)

P.S. I don't find build noise to be an issue, FWIW. The bulk of the noise is from npm, right? And that can be improved / is improving (I don't remember exactly) with recent versions of npm?

[hunterloftis]

Feature is in master; I'll leave it there for a while before publishing the official buildkit so we can work out any issues.

To test:

heroku buildpacks:clear
heroku buildpacks:set https://github.com/heroku/heroku-buildpack-nodejs

[aseemk]

Nice! Thanks @hunterloftis.

[devpascoe]

i'm with @aseemk
i just added an env var POST_INSTALL_ENV = staging
then in package.json
scripts: {
"start": "node server.js",
"postinstall": "sh postinstall.sh"
}

and in postinstall.sh

#!/bin/bash
echo "--- PostInstall script ---"
if [ "$POST_INSTALL_ENV" = "production" ]
then
echo "--- On Production ---"
else
echo "--- On Staging ---"
fi

[yagudaev]

We actually ended up creating a

NODE_ENV=production
BROWSER_ENV=staging

On heroku. We expose it as

window.BROWSER_ENV
// on node
process.env.BROWSER_ENV