From af98ec73760e6f0ef64a8e032e825a615f187e35 Mon Sep 17 00:00:00 2001
From: Dennis Snell <dennis.snell@automattic.com>
Date: Fri, 3 Mar 2017 14:52:10 -0700
Subject: [PATCH] config(): Add safety falback for when running in production
 (#11686)

* config(): Add safety falback for when running in production

Recently the data layer experienced a total failure when #11490 merged
in and crashed when it failed to initialize the new middleware handler.

The fact that a key was requested for which no value existed led to
`config()` raising an `Error` which causes the entire middleware chain
to fail to build.

Now `config()` has been modified to only raise an actual `Error` when
the `NODE_ENV=developement` and instead in all other environments to
simply log a message to the browser console and return `undefined`. This
should allow us to still catch any mistakes by the obvious message while
preventing such otherwise-small errors from cascading out-of-control.
---
 server/config/index.js       | 55 +++++++++++++++++++++++++++++++-----
 server/config/test/config.js | 41 +++++++++++++++++++++++++++
 2 files changed, 89 insertions(+), 7 deletions(-)
 create mode 100644 server/config/test/config.js

diff --git a/server/config/index.js b/server/config/index.js
index 9a31db1074ac0d..d75a70bc510c8c 100644
--- a/server/config/index.js
+++ b/server/config/index.js
@@ -1,4 +1,6 @@
-const configPath = require( 'path' ).resolve( __dirname, '..', '..', 'config' );
+const path = require( 'path' );
+
+const configPath = path.resolve( __dirname, '..', '..', 'config' );
 const data = require( './parser' )( configPath, {
 	env: process.env.CALYPSO_ENV || process.env.NODE_ENV || 'development',
 	includeSecrets: true,
@@ -7,18 +9,57 @@ const data = require( './parser' )( configPath, {
 } );
 
 /**
- * Return config `key`.
- * Throws an error if the requested `key` is not set in the config file.
+ * Returns configuration value for given key
+ *
+ * If the requested key isn't defined in the configuration
+ * data then this will report the failure with either an
+ * error or a console warning.
+ *
+ * When in the 'development' NODE_ENV it will raise an error
+ * to crash execution early. However, because many modules
+ * call this function in the module-global scope a failure
+ * here can not only crash that module but also entire
+ * application flows as well as trigger unexpected and
+ * unwanted behaviors. Therefore if the NODE_ENV is not
+ * 'development' we will return `undefined` and log a message
+ * to the console instead of halting the execution thread.
+ *
+ * The config files are loaded in sequence: _shared.json, {env}.json, {env}.local.json
+ * @see server/config/parser.js
  *
- * @param {String} key The key of the config entry.
- * @return {Mixed} Value of config or error if not found.
- * @api public
+ * @throws {ReferenceError} when key not defined in the config (NODE_ENV=development only)
+ * @param {String} key name of the property defined in the config files
+ * @returns {*} value of property named by the key
  */
 function config( key ) {
 	if ( key in data ) {
 		return data[ key ];
 	}
-	throw new Error( 'config key `' + key + '` does not exist' );
+
+	if ( 'development' === process.env.NODE_ENV ) {
+		throw new ReferenceError(
+			`Could not find config value for key '${ key }'\n` +
+			`Please make sure that if you need it then it has a default value assigned in 'config/_shared.json'`
+		);
+	}
+
+	// display console error only in a browser
+	// (not in tests, for example)
+	if ( 'undefined' !== typeof window ) {
+		console.error(
+			`%cCore Error: ` +
+			`%cCould not find config value for key %c${ key }%c. ` +
+			`Please make sure that if you need it then it has a default value assigned in ` +
+			`%cconfig/_shared.json` +
+			`%c.`,
+			'color: red; font-size: 120%', // error prefix
+			'color: black;', // message
+			'color: blue;', // key name
+			'color: black;', // message
+			'color: blue;', // config file reference
+			'color: black' // message
+		);
+	}
 }
 
 function isEnabled( feature ) {
diff --git a/server/config/test/config.js b/server/config/test/config.js
new file mode 100644
index 00000000000000..b4db71bfc02cf5
--- /dev/null
+++ b/server/config/test/config.js
@@ -0,0 +1,41 @@
+/**
+ * External dependencies
+ */
+import { expect } from 'chai';
+
+/**
+ * Internal dependencies
+ */
+import config from '../';
+
+const NODE_ENV = process.env.NODE_ENV;
+const fakeKey = 'where did all the errors go?';
+
+describe( '#config', () => {
+	afterEach( () => process.env.NODE_ENV = NODE_ENV );
+
+	it( `should throw an error when given key doesn't exist (NODE_ENV == development)`, () => {
+		process.env.NODE_ENV = 'development';
+
+		expect( () => config( fakeKey ) ).to.throw( ReferenceError );
+	} );
+
+	it( `should not throw an error when given key doesn't exist (NODE_ENV != development)`, () => {
+		const envs = [
+			'client',
+			'desktop',
+			'horizon',
+			'production',
+			'stage',
+			'test',
+			'wpcalypso',
+		];
+
+		envs.forEach( env => {
+			process.env.NODE_ENV = env;
+
+			expect( process.env.NODE_ENV ).to.equal( env );
+			expect( () => config( fakeKey ) ).to.not.throw( Error );
+		} );
+	} );
+} );