Skip to content
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

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

Merged
merged 2 commits into from
Mar 3, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 38 additions & 7 deletions server/config/index.js
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -7,18 +9,47 @@ 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.
*
* @param {String} key The key of the config entry.
* @return {Mixed} Value of config or error if not found.
* @api public
* The config files are loaded in sequence: _shared.json, {env}.json, {env}.local.json
* @see server/config/parser.js
*
* @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' );

const errorMessage = (
`Could not find config value for key '${ key }'\n` +
`Please make sure that if you need it then it has a value assigned in 'config/_shared.json'`
);

if ( 'development' === process.env.NODE_ENV ) {
throw new ReferenceError( errorMessage );
}

// display console error only in a browser
// (not in tests, for example)
if ( 'undefined' !== typeof window ) {
console.error( errorMessage );
Copy link
Contributor

@aduth aduth Mar 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the above thrown error log to console? Or, if not, would it be enough to move console.log into the NODE_ENV condition (since tests have their own NODE_ENV)?

Copy link
Member Author

@dmsnell dmsnell Mar 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what you mean @aduth - they both log to the console but logging to the console isn't the problem. the thrown error halts the execution of the current JavaScript thread and that causes the big problem because it kills code which does nothing more than import the broken modules

it's my belief right now that we want to make an annoying console message so that any of us can quickly identify the failures in production but we want to prevent those failures from cascading outward thus removing the error

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's my belief right now that we want to make an annoying console message so that any of us can quickly identify the failures in production but we want to prevent those failures from cascading outward thus removing the error

Makes sense 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add in some styling to differentiate our Calypso framework warnings from React errors? For example:

console.error( `%c ${ error Message }`, 'color:white; background:black;' );

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in f987c75 I added some styling @gwwar - wha'd'ya think?

screen shot 2017-03-01 at 9 06 14 pm

}
}

function isEnabled( feature ) {
Expand Down
41 changes: 41 additions & 0 deletions server/config/test/config.js
Original file line number Diff line number Diff line change
@@ -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 );
} );
} );
} );