Enforcing set environment variables

Question

Context

I have a node.js application that uses many other modules and libraries. Some of these modules pull details from process environment variables, such as URL's for databases, credentials, and so on. In particular, because this node app is running in Bluemix, there's a large JSON variable called VCAP_SERVICES that contains all the connection information the app may need (users/passwords, names, conditions, and target URL's).

When the application is running in the "real" Bluemix environment, it will have these variables set. But, when the application is running in development, or debug mode it is convenient to set these variables in a more friendly way other than specifying multiple complicated, large variables.

To solve this problem, I have created a config.js script which looks in a specific folder for files with a specific extension .env. If a file is found with that extension, the environment variable with that file's name is inspected. If the variable is set, it is left alone. If the variable is not set, it is set to have the value of the file's contents.

This has to happen synchronously because this work has to complete before other modules have loaded.

To help things along, and to simplify the code management process, I have put this all in to a sub-folder, and created a .gitignore file that ignores all files with the extension .env. This prevents any passwords or other tokens from being committed to the source code repository.

Here's an example context of how the code would be used:

/*jshint node:true*/

//------------------------------------------------------------------------------
// node.js starter application for Bluemix
//------------------------------------------------------------------------------

// ensure all environment variables are set appropriately
require('./config/config.js');

// This application uses express as it's web server
// for more info, see: http://expressjs.com
var express = require('express');
var logger = require('morgan');

.......

Some example files that are used for configuration are:

PORT.env

3000

The above file will set the process.env["PORT"] to 3000 unless it was already set.

VCAP_SERVICES.env

{
  "elephantsql": [
    {
      "name": "elephantsql-c6c60",
      "label": "elephantsql",
      "tags": [
        "postgres",
        "postgresql",
        "relational"
      ],
      "plan": "turtle",
      "credentials": {
        "uri": "postgres://seilbmbd:[email protected]:5432/seilbmbd"
      }
    }
  ],
  "sendgrid": [
    {
      "name": "mysendgrid",
      "label": "sendgrid",
      "tags": [
        "smtp"
      ],
      "plan": "free",
      "credentials": {
        "hostname": "smtp.sendgrid.net",
        "username": "QvsXMbJ3rK",
        "password": "HCHMOYluTv"
      }
    }
  ]
}

The above will set the environment variable to that complicated JSON.... you can understand why setting it on a commandline would be messy...;-) ?

Implementation

With such a big background, the implementation itself is actually quite simple.... but it is critical I get this right as I learn how the node.js systems fit together.

I am looking for a review of any and all aspects of the code, the use of the libraries, the conformance with best practices (async is not an option here, I don't believe, but I am possibly wrong, please correct if I am).

'use strict';

/*
 * Load various pre-determined environment variables (files in this folder with .env extension).
 * Only if they have not previously been set in the environment.
 * 
 * This makes the setting of Bluemix style variables quite easy.
 */

var fs = require('fs');
var path = require('path');

var cfgfiles = fs.readdirSync(__dirname);

for (var i = 0; i < cfgfiles.length; i++) {
    var file = cfgfiles[i];
    if (!file.match(/\.env$/)) {
        continue;
    }
    
    var name = file;
    //console.log("processing file " + file);
    name = name.replace(/\.env$/, "");
    file = path.join(__dirname , file);
    if (process.env[name]) {
        //console.log("Environment variable " + name + " already set: ignoring");
    } else {
        var val = fs.readFileSync(file, 'utf8');
        process.env[name] = val;
        console.log("Loaded unset environment variable " + name + " from file " + file);
        //console.log("setting environment " + name + " to config file " + file + ":\n   " + val);
    }
}

Answer 1

One simple thing I see as a (small performance) improvement would be to store the length of your set of files.

You have this tiny for:

for (var i = 0; i < cfgfiles.length; i++) {

Try to replace it with:

for (var i = 0, length = cfgfiles.length; i < length; i++) {

This speeds up the code because you don't have to access to the length property in the object. Accessing to properties in objects is always slower than accessing to local variables.

---

A few lines below, you have this:

if (process.env[name]) {
    //console.log("Environment variable " + name + " already set: ignoring");
} else {
    var val = fs.readFileSync(file, 'utf8');
    process.env[name] = val;
    console.log("Loaded unset environment variable " + name + " from file " + file);
    //console.log("setting environment " + name + " to config file " + file + ":\n   " + val);
}

Besides the comments being there, which are useless to those who try to read the code, you have a completely useless if:

if (process.env[name]) {
    //console.log("Environment variable " + name + " already set: ignoring");
}

Which does absolutely nothing.

You could use it to set a new value or remove it entirely.

Also, the way you check the value is incorrect. What if the value is 0? It will be re-written! What if it is '<empty string>'? It won't! There you go, you have a bug/inconsistent behaviour.

Rewrite it to something like this:

if (!(name in process.env)) {
    var val = fs.readFileSync(file, 'utf8');
    process.env[name] = val;
    console.log("Loaded unset environment variable " + name + " from file " + file);
}

As an alternative, you can use this if:

if (!process.env.hasOwnProperty(name)) {

The difference is that this method will check if a determined property exists within the object's own properties (the ones you provided). Inherited properties from the Object prototype would return false here. This is only a concern if you have a file called toString or constructor or hasOwnProperty. Depending on what you are trying to do, you may not care about those methods (except the last one).

---

Also, you have a lack of comments here:

name = name.replace(/\.env$/, "");
file = path.join(__dirname , file);

So, what are you trying to do? It took me 10 minutes to understand what was going on there. A comment would have been nice.

---

Also, as a small nitpicky point, you have the regexp /\.env$/ being repeated. You should store it in a variable. Also, consider adding the i flag, so that abc.ENV can still be matched.

---

As a final note, and not knowing how node.js works in this regard, you could try to wrap your code in an anonymous function. This avoids leaking variables everywhere once you are done. But I don't know how it would work or not. If it indeed worked. For 'regular' Javascript, I use the following:

(function(window, undefined) {
    [code]
})(Function('return this')());

Which isn't the most readable thing, but works as intended.

Answer 2

Comments can be useful.

But to me, leaving in commented lines of code usually just looks like clutter.

You have this:

//console.log("processing file " + file);

and this:

//console.log("Environment variable " + name + " already set: ignoring");

and this:

//console.log("setting environment " + name + " to config file " + file + ":\n   " + val);

Both have been left in the source code without explanation.

Source control should help you keep track of code that used to be there, so there's not a real good excuse to leave it there for any historical reason.

Arguably, you might want to leave it in if it's something you're frequently uncommenting for some sort of testing purposes, but if that's the case, perhaps leave a comment above the line, something to the effect of:

// Uncomment the following line to ... 

With a description of whatever is achieved by uncommenting that line.

Otherwise, as I said at the top, it tends to look like clutter (at least to me).

If I'm maintaining this code, I don't know why it's there, so I probably just delete it.