From 4728d5e4469cd98fe9a06ac60ad62c63852fe4f7 Mon Sep 17 00:00:00 2001 From: Mark Striemer Date: Wed, 4 May 2016 15:31:07 -0500 Subject: [PATCH 1/3] Log with bunyan (fixes #182) --- config/default.js | 5 ++++ config/development.js | 3 ++ package.json | 24 ++-------------- src/core/server/base.js | 49 +++++++++++++++++++++------------ src/core/store.js | 10 +++++++ src/disco/store.js | 6 ++-- src/search/store.js | 6 ++-- tests/client/core/test_store.js | 21 ++++++++++++++ 8 files changed, 79 insertions(+), 45 deletions(-) create mode 100644 src/core/store.js create mode 100644 tests/client/core/test_store.js diff --git a/config/default.js b/config/default.js index 547e0e669ff..a13437c5fa8 100644 --- a/config/default.js +++ b/config/default.js @@ -24,6 +24,9 @@ module.exports = { cookieMaxAge: 2592000, cookieName: 'jwt_api_auth_token', + isDeployed: true, + isDevelopment: false, + // The canonical list of enabled apps. validAppNames, @@ -46,6 +49,8 @@ module.exports = { 'apiBase', 'cookieName', 'cookieMaxAge', + 'isDeployed', + 'isDevelopment', 'startLoginUrl', ], diff --git a/config/development.js b/config/development.js index cf5d013e209..03985babdb0 100644 --- a/config/development.js +++ b/config/development.js @@ -8,6 +8,9 @@ module.exports = { apiHost: 'https://addons-dev.allizom.org', amoCDN: 'https://addons-dev-cdn.allizom.org', + isDeployed: false, + isDevelopment: true, + webpackServerHost: '127.0.0.1', webpackServerPort: 3001, webpackHost: defer((cfg) => `http://${cfg.webpackServerHost}:${cfg.webpackServerPort}`), diff --git a/package.json b/package.json index f8960853dcf..99fc054932f 100644 --- a/package.json +++ b/package.json @@ -14,8 +14,8 @@ "eslint": "npm run lint", "lint": "eslint .", "servertest": "npm run build && better-npm-run servertest", - "start:disco": "better-npm-run start:disco", - "start:search": "better-npm-run start:search", + "start:disco": "npm run version-check && NODE_PATH='./:./src' NODE_APP_INSTANCE=disco node bin/server.js", + "start:search": "npm run version-check && NODE_PATH='./:./src' NODE_APP_INSTANCE=search node bin/server.js", "test": "better-npm-run test", "unittest": "better-npm-run unittest", "unittest:dev": "better-npm-run unittest:dev", @@ -68,24 +68,6 @@ "NODE_ENV": "production" } }, - "start": { - "command": "npm run version-check && node bin/server.js", - "env": { - "NODE_PATH": "./:./src" - } - }, - "start:search": { - "command": "better-npm-run start", - "env": { - "NODE_APP_INSTANCE": "search" - } - }, - "start:disco": { - "command": "better-npm-run start", - "env": { - "NODE_APP_INSTANCE": "disco" - } - }, "test": { "command": "npm run version-check && npm run unittest && npm run servertest && npm run lint", "env": { @@ -127,9 +109,9 @@ "homepage": "https://github.com/mozillla/addons-frontend#readme", "dependencies": { "better-npm-run": "0.0.8", + "bunyan": "1.8.1", "camelcase": "2.1.1", "classnames": "2.2.5", - "common-tags": "0.0.3", "config": "1.20.1", "express": "4.13.4", "extract-text-webpack-plugin": "1.0.1", diff --git a/src/core/server/base.js b/src/core/server/base.js index 7b23d3b3548..2eb2a18b1a8 100644 --- a/src/core/server/base.js +++ b/src/core/server/base.js @@ -6,6 +6,7 @@ import cookie from 'react-cookie'; import React from 'react'; import ReactDOM from 'react-dom/server'; import ReactHelmet from 'react-helmet'; +import bunyan from 'bunyan'; import { Provider } from 'react-redux'; import { match } from 'react-router'; @@ -19,20 +20,37 @@ import config from 'config'; import { setJWT } from 'core/actions'; const env = config.util.getEnv('NODE_ENV'); -const isDeployed = ['stage', 'dev', 'production'].indexOf(env) > -1; +const isDeployed = config.get('isDeployed'); +const isDevelopment = config.get('isDevelopment'); const errorString = 'Internal Server Error'; const appName = config.get('appName'); +const log = bunyan.createLogger({ + name: 'server', + app: appName, + serializers: bunyan.stdSerializers, +}); + // Globals (these are set by definePlugin for client-side builds). global.CLIENT = false; global.SERVER = true; -global.DEVELOPMENT = env === 'development'; +global.DEVELOPMENT = isDevelopment; + +function logRequests(req, res, next) { + const start = new Date(); + next(); + const finish = new Date(); + const elapsed = finish - start; + log.info({req, res, start, finish, elapsed}); +} function baseServer(routes, createStore, { appInstanceName = appName } = {}) { const app = new Express(); app.disable('x-powered-by'); + app.use(logRequests); + // Sets X-Frame-Options app.use(helmet.frameguard('deny')); @@ -45,8 +63,8 @@ function baseServer(routes, createStore, { appInstanceName = appName } = {}) { // CSP configuration. app.use(helmet.contentSecurityPolicy(config.get('CSP'))); - if (env === 'development') { - console.log('Running in Development Mode'); // eslint-disable-line no-console + if (isDevelopment) { + log.info('Running in Development Mode'); // clear require() cache if in development mode webpackIsomorphicTools.refresh(); @@ -67,7 +85,7 @@ function baseServer(routes, createStore, { appInstanceName = appName } = {}) { cookie.plugToRequest(req, res); if (err) { - console.error(err); // eslint-disable-line no-console + log.error({err, req}); return res.status(500).end(errorString); } @@ -109,8 +127,7 @@ function baseServer(routes, createStore, { appInstanceName = appName } = {}) { res.send(`${HTML}`); }) .catch((error) => { - // eslint-disable-next-line no-console - console.error(error.stack); + log.error({err: error}); res.status(500).end(errorString); }); }); @@ -118,8 +135,7 @@ function baseServer(routes, createStore, { appInstanceName = appName } = {}) { // eslint-disable-next-line no-unused-vars app.use((err, req, res, next) => { - // eslint-disable-next-line no-console - console.error(err.stack); + log.error({err}); res.status(500).end(errorString); }); @@ -128,8 +144,7 @@ function baseServer(routes, createStore, { appInstanceName = appName } = {}) { export function runServer({listen = true, app = appName} = {}) { if (!app) { - // eslint-disable-next-line no-console - console.log(`Please specify a valid appName from ${config.get('validAppNames')}`); + log.fatal(`Please specify a valid appName from ${config.get('validAppNames')}`); process.exit(1); } @@ -137,7 +152,7 @@ export function runServer({listen = true, app = appName} = {}) { const host = config.get('serverHost'); const isoMorphicServer = new WebpackIsomorphicTools(WebpackIsomorphicToolsConfig); - return isoMorphicServer.development(env === 'development') + return isoMorphicServer.development(isDevelopment) .server(config.get('basePath')) .then(() => { global.webpackIsomorphicTools = isoMorphicServer; @@ -154,10 +169,9 @@ export function runServer({listen = true, app = appName} = {}) { if (err) { reject(err); } - // eslint-disable-next-line no-console - console.log(`🔥 Addons-frontend server is running [ENV:${env}] [APP:${app}]`); - // eslint-disable-next-line no-console - console.log(`👁 Open your browser at http://${host}:${port} to view it.`); + log.info(`🔥 Addons-frontend server is running [ENV:${env}] [APP:${app}] ` + + `[isDevelopment:${isDevelopment}] [isDeployed:${isDeployed}]`); + log.info(`👁 Open your browser at http://${host}:${port} to view it.`); resolve(server); }); } else { @@ -166,8 +180,7 @@ export function runServer({listen = true, app = appName} = {}) { }); }) .catch((err) => { - // eslint-disable-next-line no-console - console.error(err); + log.error({err}); }); } diff --git a/src/core/store.js b/src/core/store.js new file mode 100644 index 00000000000..fceff693921 --- /dev/null +++ b/src/core/store.js @@ -0,0 +1,10 @@ +import { applyMiddleware } from 'redux'; +import createLogger from 'redux-logger'; +import config from 'config'; + +export function middleware({ __config = config } = {}) { + if (__config.get('isDevelopment')) { + return applyMiddleware(createLogger()); + } + return undefined; +} diff --git a/src/disco/store.js b/src/disco/store.js index 1d7fba679ff..1a930a685c1 100644 --- a/src/disco/store.js +++ b/src/disco/store.js @@ -1,11 +1,11 @@ -import { applyMiddleware, createStore as _createStore, combineReducers } from 'redux'; +import { createStore as _createStore, combineReducers } from 'redux'; import { reducer as reduxAsyncConnect } from 'redux-async-connect'; -import createLogger from 'redux-logger'; +import { middleware } from 'core/store'; export default function createStore(initialState = {}) { return _createStore( combineReducers({reduxAsyncConnect}), initialState, - applyMiddleware(createLogger()), + middleware(), ); } diff --git a/src/search/store.js b/src/search/store.js index b27dffc006c..5c706dbae89 100644 --- a/src/search/store.js +++ b/src/search/store.js @@ -1,16 +1,16 @@ -import { applyMiddleware, createStore as _createStore, combineReducers } from 'redux'; +import { createStore as _createStore, combineReducers } from 'redux'; import { reducer as reduxAsyncConnect } from 'redux-async-connect'; -import createLogger from 'redux-logger'; import addons from 'core/reducers/addons'; import api from 'core/reducers/api'; import auth from 'core/reducers/authentication'; import search from 'search/reducers/search'; +import { middleware } from 'core/store'; export default function createStore(initialState = {}) { return _createStore( combineReducers({addons, api, auth, search, reduxAsyncConnect}), initialState, - applyMiddleware(createLogger()), + middleware(), ); } diff --git a/tests/client/core/test_store.js b/tests/client/core/test_store.js new file mode 100644 index 00000000000..e14e69aa8d0 --- /dev/null +++ b/tests/client/core/test_store.js @@ -0,0 +1,21 @@ +import { middleware } from 'core/store'; + +describe('core store middleware', () => { + it('includes the middleware in development', () => { + const config = { + get() { + return true; + }, + }; + assert.isFunction(middleware({__config: config})); + }); + + it('is undefined when not in development', () => { + const config = { + get() { + return false; + }, + }; + assert.strictEqual(undefined, middleware({__config: config})); + }); +}); From 90a97afde23cfcd988225025da7f6ba827238ff0 Mon Sep 17 00:00:00 2001 From: Mark Striemer Date: Thu, 5 May 2016 14:53:06 -0500 Subject: [PATCH 2/3] Remove unused globals --- .eslintrc | 3 --- karma.conf.js | 3 --- src/core/server/base.js | 5 ----- webpack.dev.config.babel.js | 3 --- webpack.prod.config.babel.js | 3 --- 5 files changed, 17 deletions(-) diff --git a/.eslintrc b/.eslintrc index c2df7a28093..59cb6f27b7f 100644 --- a/.eslintrc +++ b/.eslintrc @@ -4,10 +4,7 @@ }, "extends": "airbnb", "globals": { - "CLIENT": true, "CLIENT_CONFIG": true, - "DEVELOPMENT": true, - "SERVER": true, "assert": true, "sinon": true, "webpackIsomorphicTools": true diff --git a/karma.conf.js b/karma.conf.js index 26261deec74..d7a28b1b365 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -20,9 +20,6 @@ const coverageReporters = [{ const newWebpackConfig = Object.assign({}, webpackConfigProd, { plugins: [ new webpack.DefinePlugin({ - DEVELOPMENT: false, - CLIENT: true, - SERVER: false, CLIENT_CONFIG: JSON.stringify(clientConfig), }), new webpack.NormalModuleReplacementPlugin(/config$/, 'client-config.js'), diff --git a/src/core/server/base.js b/src/core/server/base.js index 2eb2a18b1a8..5181c6bc12b 100644 --- a/src/core/server/base.js +++ b/src/core/server/base.js @@ -32,11 +32,6 @@ const log = bunyan.createLogger({ serializers: bunyan.stdSerializers, }); -// Globals (these are set by definePlugin for client-side builds). -global.CLIENT = false; -global.SERVER = true; -global.DEVELOPMENT = isDevelopment; - function logRequests(req, res, next) { const start = new Date(); next(); diff --git a/webpack.dev.config.babel.js b/webpack.dev.config.babel.js index 65027cfc8aa..a3219b9bb13 100644 --- a/webpack.dev.config.babel.js +++ b/webpack.dev.config.babel.js @@ -74,10 +74,7 @@ export default Object.assign({}, webpackConfig, { }, plugins: [ new webpack.DefinePlugin({ - CLIENT: true, CLIENT_CONFIG: JSON.stringify(clientConfig), - DEVELOPMENT: true, - SERVER: false, }), new webpack.NormalModuleReplacementPlugin(/config$/, 'client-config.js'), new webpack.HotModuleReplacementPlugin(), diff --git a/webpack.prod.config.babel.js b/webpack.prod.config.babel.js index 13249547d64..71c6c885d3b 100644 --- a/webpack.prod.config.babel.js +++ b/webpack.prod.config.babel.js @@ -49,9 +49,6 @@ export default { }, plugins: [ new webpack.DefinePlugin({ - DEVELOPMENT: false, - CLIENT: true, - SERVER: false, CLIENT_CONFIG: JSON.stringify(clientConfig), }), // Replaces server config module with the subset clientConfig object. From 37812a17d4acc0be2a733804f67765dd5e6bac94 Mon Sep 17 00:00:00 2001 From: Mark Striemer Date: Thu, 5 May 2016 14:59:08 -0500 Subject: [PATCH 3/3] Add a basic npm start that uses NODE_APP_INSTANCE --- package.json | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index 99fc054932f..3c60960660e 100644 --- a/package.json +++ b/package.json @@ -14,8 +14,9 @@ "eslint": "npm run lint", "lint": "eslint .", "servertest": "npm run build && better-npm-run servertest", - "start:disco": "npm run version-check && NODE_PATH='./:./src' NODE_APP_INSTANCE=disco node bin/server.js", - "start:search": "npm run version-check && NODE_PATH='./:./src' NODE_APP_INSTANCE=search node bin/server.js", + "start": "npm run version-check && NODE_PATH='./:./src' node bin/server.js", + "start:disco": "NODE_APP_INSTANCE=disco npm start", + "start:search": "NODE_APP_INSTANCE=search npm start", "test": "better-npm-run test", "unittest": "better-npm-run unittest", "unittest:dev": "better-npm-run unittest:dev",