Fixed env variable in lib/config.js

[shaunanoordin]

Bugfix: 'env' variable in config.js was returning invalid values.

Symptoms:

• When using the Panoptes client in apps, the app will sometimes - depending on the process environment - throw an error, notably Uncaught SugarClient.host is not defined
• See: Add async wildcam-gorongosa-education#22 (comment)

Analysis:

• We've detected that the env variable in lib/config.js doesn't always have one the valid values, i.e. 'production', 'staging' or 'cam'.
• In certain environments, the value of env is 'development' or something similarly invalid. This is the result of different Node setups whent this line is called:

var envFromShell = process.env.NODE_ENV;

• This will cause a chain reaction with the rest of the code...

//...
var envFromBrowser = locationMatch(/\W?env=(\w+)/);
var envFromShell = process.env.NODE_ENV;
var env = envFromBrowser || envFromShell || DEFAULT_ENV;

module.exports = {
  host: hostFromBrowser || hostFromShell || API_HOSTS[env],  //env can be invalid here
  clientAppID: appFromBrowser || appFromShell || API_APPLICATION_IDS[env],  //and here
  talkHost: talkFromBrowser || talkFromShell || TALK_HOSTS[env],  //etc
  sugarHost: sugarFromBrowser || sugarFromShell || SUGAR_HOSTS[env],
};
//...

Actions:

• Recommendation: add additional fallbacks or a value whitelist so the env var cannot be invalid.
• This PR adds API_HOSTS[DEFAULT_ENV](and similar) to the module.exports block as a fallback.

@brian-c , can you please take a look at this? Also: thanks to @rogerhutchings for shaping this fix.

[brian-c]

Seems reasonable, but would it actually be safer to explicitly throw an error if we don't recognize the environment?

Where is it getting set to "development" on your machine? Is that a default somewhere?

[shaunanoordin]

Unfortunately, I'm unable to determine what's setting my NODE_ENV variable to 'development', but I know it's the default for my system. (One month old Mac with a fresh dev environment.) @rogerhutchings mentioned other cases where NODE_ENV was set to 'undefined' - as in a 9-character string - so the wonky variable value may not be an isolated problem. (However, I'll need access to way more systems to test this out.)

I see the merits in explicitly throwing an error, but at the moment, my experience when encountering this issue threw me for a loop. The error I received in my console was Uncaught SugarClient.host is not defined, which I would never have guessed would relate to the Panoptes Client if Roger didn't share his knowledge on the matter.

This PR implements a failsafe fallback, but I can imagine an alternative that implements a... fail-unsafe? faildangerous? failhard?... sanity check with better debug info.

if (!env.match(/^(production|staging|cam)$/)) { 
  throw 'Panoptes Client Error: Invalid Environment, try setting your NODE_ENV to something that's not "'+envFromShell+'" or add "?env=staging" to your browser's URL.';
}

I'm happy with any solution as long as Panoptes Client-using devs don't go "Bwuh?" when they have an invalid NODE_ENV.

Coming to cinemas this summer: Fail Hard, starring Bruce Willis

[brian-c]

Yeah, I think an error is the way to go. Probably leave out the query param hack though, it's for quick switching, not a long-term solution.

[brian-c]

Closed by #23.