Merge pull request #640 from Financial-Times/rollBackv22Commits

Roll back v22 breaking changes

cert.pem

@@ -18,4 +18,4 @@ Srxu4nSvKRXQxidB9vgNwOdSn1p+oIeVIotnikhfReITUlAftFUxFzZnIIalmCCE
 ljtgLJGAFdshVIyrIddGjGU3YsygesA5BVf+ri6TQ8/Obukg5ifDAUyHBkItzGbc
 EdRNX3JKBpm/laFEAVA0Dno6Bcx4n7dNjug+CEq343vSsMMKE/xi3C34D/t4FKgN
 Scah7m7uiE+e7SvIq6hvZwgjREg///iBSyAvGp5X5rqmTS2h5pMrvw==
------END CERTIFICATE-----
+-----END CERTIFICATE-----

key.pem

@@ -24,4 +24,4 @@ F0Fs0M4L/fWGxyaxn1YNYJxpI1gxWGioGHMIpHfzVO05Ww2vPNVpjvS/BHXFkwGh
 Cfr/AoGAEVfE+pHlAlbEo+DtbRcig4qmf1Ja+kX7dwI/C+AbliTxZWMwFXzZREhF
 /Uni45pdn9EYC4DbKxKE92oeBS9oq3ZWP15fDCaxPuoiqWf0vEHe7Dr59PHgFyGx
 XKzraQa+YGEmLqnJQYSvwfoIFKF32UUBmvo929COTka4Ki6ACJ0=
------END RSA PRIVATE KEY-----
+-----END RSA PRIVATE KEY-----

package.json

@@ -11,15 +11,15 @@
     "prepare": "npx snyk protect || npx snyk protect -d || true"
   },
   "dependencies": {
-    "@financial-times/n-flags-client": "^9.1.5",
-    "@financial-times/n-logger": "^6.0.0",
-    "@financial-times/n-raven": "^3.0.3",
+    "@financial-times/n-flags-client": "^10.0.0",
+    "@financial-times/n-logger": "^8.0.0",
+    "@financial-times/n-raven": "^5.0.0",
     "debounce": "^1.1.0",
     "denodeify": "^1.2.1",
     "express": "^4.16.3",
-    "isomorphic-fetch": "^2.2.1",
+    "isomorphic-fetch": "^3.0.0",
     "n-health": "^5.0.1",
-    "next-metrics": "^5.0.20"
+    "next-metrics": "^5.0.23"
   },
   "devDependencies": {
     "@financial-times/n-gage": "^3.12.0",

src/lib/error-rate-check.js

@@ -21,7 +21,7 @@ module.exports = (appName, opts) => {
 		samplePeriod,
 		severity,
 		businessImpact: 'Users may see application error pages.',
-		technicalSummary: `The proportion of error responses for ${appName} is greater than 4% of all responses. This is a default n-express check.`,
+		technicalSummary: `The proportion of error responses for ${appName} is greater than ${threshold}% of all responses. This is a default n-express check.`,
 		panicGuide: 'Consult errors in sentry, application logs in splunk and run the application locally to identify errors'
 	});
 };

src/lib/instrument-listen.js

@@ -5,66 +5,57 @@ const http = require('http');
 const https = require('https');
 const denodeify = require('denodeify');
 const path = require('path');
-
 const fs = require('fs');
-const readFile = denodeify(fs.readFile);
 
 module.exports = (app, meta, initPromises) => {
-	async function createServer(app) {
+
+	const actualAppListen = function () {
+		let serverPromise;
 		if (process.argv.indexOf('--https') > -1) {
-			const [key, cert] = await Promise.all([
+			const readFile = denodeify(fs.readFile);
+			serverPromise = Promise.all([
 				readFile(path.resolve(__dirname, '../../key.pem')),
 				readFile(path.resolve(__dirname, '../../cert.pem'))
 			])
-
-			return https.createServer({ key, cert }, app);
+				.then(results => https.createServer({ key: results[0], cert: results[1] }, this));
 		} else {
-			return http.createServer(app);
+			serverPromise = Promise.resolve(http.createServer(this));
 		}
-	}
-
-	async function actualAppListen(app, ...args) {
-		const server = await createServer(app)
 
-		return server.listen(...args);
+		return serverPromise.then(server => server.listen.apply(server, arguments));
 	};
 
-	app.listen = async function (port, callback) {
-		// these middleware are attached in .listen so they're
-		// definitely after any middleware added by the app itself
+	app.listen = function () {
+		const args = [].slice.apply(arguments);
 
 		// The error handler must be before any other error middleware
 		app.use(raven.errorHandler());
 
 		// Optional fallthrough error handler
-		app.use((err, req, res, next) => { // eslint-disable-line no-unused-vars
+		app.use((err, req, res, next) => { //eslint-disable-line
 			// The error id is attached to `res.sentry` to be returned
 			// and optionally displayed to the user for support.
 			res.statusCode = 500;
 			res.end(res.sentry + '\n');
 		});
 
-		function wrappedCallback() {
+		const port = args[0];
+		const cb = args[1];
+		args[1] = function () {
 			// HACK: Use warn so that it gets into Splunk logs
 			nLogger.warn({ event: 'EXPRESS_START', app: meta.name, port: port, nodeVersion: process.version });
-
-			if(callback) {
-				return callback.apply(this, arguments);
-			}
+			return cb && cb.apply(this, arguments);
 		};
 
-		try {
-			await Promise.all(initPromises)
-		} catch(err) {
-			// crash app if initPromises fail by throwing an error asynchronously outside of the promise
-			// TODO: better error handling
-			setTimeout(() => {
+		return Promise.all(initPromises)
+			.then(() => {
+				metrics.count('express.start');
+				return actualAppListen.apply(app, args);
+			})
+			// Crash app if initPromises fail
+			.catch(err => setTimeout(() => {
 				throw err;
-			});
-		}
-
-		metrics.count('express.start');
-		return actualAppListen(app, port, wrappedCallback);
+			}));
 	};
 
 	return app;

test/app/app.test.js

@@ -1,7 +1,6 @@
 /*global it, GLOBAL, describe, beforeEach, before, after*/
 const path = require('path');
 const request = require('supertest');
-
 // stub the setup api calls
 const fetchMock = require('fetch-mock');
 const metrics = require('next-metrics');
@@ -10,91 +9,72 @@ const nextExpress = require('../../main');
 const expect = require('chai').expect;
 const raven = require('@financial-times/n-raven');
 const flags = require('@financial-times/n-flags-client');
-
 let app;
-
 describe('simple app', function () {
-
 	before(() => {
-
 		fetchMock
 			.mock(/next-flags-api\.ft\.com/, [])
 			.mock('http://ft-next-health-eu.herokuapp.com/failure-simulation-config', {failures: []})
 			.catch(200);
-
 		app = require('../fixtures/app/main');
-
 		fetchMock.restore();
 	});
-
 	it('should have its own route', function (done) {
 		request(app)
 			.get('/')
 			.expect('Vary', /FT-Flags/i)
 			.expect(200, 'Hello world', done);
 	});
-
 	it('should be possible to add routers', function (done) {
 		request(app)
 			.get('/router/')
 			.expect('Vary', /FT-Flags/i)
 			.expect(200, 'Hello router', done);
 	});
-
 	it('should have a robots.txt', function (done) {
 		request(app)
 			.get('/robots.txt')
 			.expect(200, done);
 	});
-
 	it('should have an about json', function (done) {
 		request(app)
 			.get('/__about')
 			.expect(200, done);
 	});
-
 	describe('backend access', function () {
 		before(function () {
 			process.env.NODE_ENV = 'production';
 		});
-
 		after(function () {
 			process.env.NODE_ENV = '';
 		});
-
 		it('should have no backend authentication when no keys present', function (done) {
 			request(app)
 				.get('/let-me-in')
 				.expect(200, () => {
 					done();
 				});
 		});
-
 	});
-
 	describe('metrics', function () {
-
 		beforeEach(function () {
 			delete flags.url;
 			GLOBAL.fetch.restore();
 			// fake metrics has not been initialised
 			delete metrics.graphite;
 		});
-
 		function getApp (conf) {
 			conf = conf || {};
 			conf.directory = path.resolve(__dirname, '../fixtures/app/');
 			conf.systemCode = 'test-app';
 			return nextExpress(conf);
 		}
-
 		it('should initialise metrics', function () {
 			sinon.stub(metrics, 'init');
 			getApp();
 			expect(metrics.init.calledWith({flushEvery: 40000 })).to.be.true;
 			metrics.init.restore();
 		});
-
 		it('should initialise metrics for variant apps', function () {
 			sinon.stub(metrics, 'init');
 			process.env.FT_APP_VARIANT = 'testing';
@@ -104,41 +84,43 @@ describe('simple app', function () {
 			delete process.env.FT_APP_VARIANT;
 		});
 
-		it('should count application starts', async function () {
+		it('should count application starts', function (done) {
 			sinon.stub(metrics, 'count');
-
 			const app = getApp();
-			await app.listen()
-
-			expect(metrics.count.calledWith('express.start')).to.be.true;
-			metrics.count.restore();
+			app.listen().then(function () {
+				expect(metrics.count.calledWith('express.start')).to.be.true;
+				metrics.count.restore();
+				done();
+			});
 		});
 
-		it('should instrument fetch for recognised services', async function () {
+		it('should instrument fetch for recognised services', function (done) {
 			const realFetch = GLOBAL.fetch;
 
 			sinon.stub(raven, 'captureMessage');
 			getApp();
 
 			expect(GLOBAL.fetch).to.not.equal(realFetch);
 
-			await Promise.all([
+			Promise.all([
 				fetch('http://ft-next-api-user-prefs-v002.herokuapp.com/', {
 					timeout: 50
-				}).catch(() => {}),
+				}).catch(function () {}),
 				fetch('http://bertha.ig.ft.com/ghjgjh', {
 					timeout: 50
-				}).catch(() => {})
+				}).catch(function () {})
 			])
+				.then(function () {
+					expect(raven.captureMessage.called).to.be.false;
+					raven.captureMessage.restore();
+					done();
+				});
 
-			expect(raven.captureMessage.called).to.be.false;
-			raven.captureMessage.restore();
 		});
 	});
 
 	describe('config', () => {
 		it('should be possible to disable flags', function (done) {
-
 			sinon.stub(flags, 'init').returns(Promise.resolve(null));
 			const app = nextExpress({
 				name: 'noflags',
@@ -157,7 +139,6 @@ describe('simple app', function () {
 					done();
 				});
 		});
-
 		it('should expect a system code', () => {
 			expect(() => nextExpress({
 				name: 'nosystem',
@@ -166,5 +147,4 @@ describe('simple app', function () {
 			})).to.throw('All applications must specify a Biz Ops `systemCode` to the express() function. See the README for more details.');
 		});
 	});
-
 });