From 390a6ba9119cc24bea993a0f2795cd5d1468a53f Mon Sep 17 00:00:00 2001 From: Paul Bakker Date: Mon, 21 May 2018 19:00:42 -0700 Subject: [PATCH 1/6] new: hot reload functionallity (#13) --- lib/install.js | 52 +++++++++++++++++++++++++++++++++++++++++++++++++- lib/schemas.js | 3 +++ package.json | 3 ++- 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/lib/install.js b/lib/install.js index bc00769..b986b8a 100644 --- a/lib/install.js +++ b/lib/install.js @@ -6,6 +6,9 @@ var _ = require('lodash'); var assert = require('assert-plus'); var vasync = require('vasync'); var verror = require('verror'); +var compose = require('restify').helpers.compose; +var bunyan = require('bunyan'); +var LOG; /** * Install the enroute routes into the restify server. @@ -23,6 +26,12 @@ function install(opts, cb) { assert.object(opts.server, 'opts.server'); assert.string(opts.basePath, 'opts.basePath'); + if (typeof opts.log !== 'undefined') { + LOG = opts.log.child({ component: 'enroute' }); + } else { + LOG = bunyan.createLogger({name: 'enroute'}); + } + vasync.pipeline({arg: {}, funcs: [ // Read the routes from disk and parse them as functions function getRoutes(ctx, cb1) { @@ -32,6 +41,15 @@ function install(opts, cb) { return cb1(); }); + if (opts.enroute.hotReload) { + if (typeof opts.basePath !== 'undefined') { + LOG.info({basedir: opts.basePath}, + 'Hot reloading of routes is enabled for base dir'); + } else { + LOG.info('Hot reloading of routes is enabled.'); + } + } + // go through each of the route names _.forEach(opts.enroute.routes, function (methods, routeName) { // go through each of the HTTP methods @@ -45,7 +63,12 @@ function install(opts, cb) { route = { name: routeName, method: method, - func: require(sourceFile) + func: opts.enroute.hotReload + ? function (req, resp, callback) { + reloadProxy(sourceFile, req, + resp, callback, opts); + } + : require(sourceFile) }; } catch (e) { return cb1(new verror.VError({ @@ -89,4 +112,31 @@ function install(opts, cb) { }); } +function reloadProxy(sourceFile, req, resp, callback, opts) { + if (typeof opts.basePath !== 'undefined') { + //Delete code loaded from a specific base dir + Object.keys(require.cache).forEach(function (cacheKey) { + if (cacheKey.indexOf(opts.basePath) !== -1) { + delete require.cache[cacheKey]; + } + }); + } else { + //Delete all cached entries + Object.keys(require.cache).forEach(function (cacheKey) { + delete require.cache[cacheKey]; + }); + } + + try { + var handlers = require(sourceFile); + var handlerChain = compose(handlers); + + handlerChain(req, resp, callback); + } catch (e) { + LOG.error('Uncaught error in route:'); + LOG.error(e); + callback(e); + } +} + module.exports = install; diff --git a/lib/schemas.js b/lib/schemas.js index 9b6a801..9021487 100644 --- a/lib/schemas.js +++ b/lib/schemas.js @@ -99,6 +99,9 @@ module.exports = { }, schemaVersion: { type: 'number' + }, + hotReload: { + type: 'boolean' } }, additionalProperties: false, diff --git a/package.json b/package.json index dc25362..3a36987 100644 --- a/package.json +++ b/package.json @@ -37,14 +37,15 @@ "jscs": "^3.0.7", "mocha": "^3.1.2", "nsp": "^2.6.2", - "restify": "^7.0.0", "restify-clients": "^1.4.0", "uuid": "^2.0.3" }, "dependencies": { "ajv": "^4.8.0", "assert-plus": "^1.0.0", + "bunyan": "^1.8.12", "lodash": "^4.16.4", + "restify": "^7.2.0", "vasync": "^1.6.4", "verror": "^1.6.0" } From 18a843b11db436169afc87a9222e299894cdc859 Mon Sep 17 00:00:00 2001 From: Alex Liu Date: Mon, 21 May 2018 19:33:45 -0700 Subject: [PATCH 2/6] fix: pr comments --- lib/install.js | 103 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 82 insertions(+), 21 deletions(-) diff --git a/lib/install.js b/lib/install.js index b986b8a..f2f79d1 100644 --- a/lib/install.js +++ b/lib/install.js @@ -8,13 +8,17 @@ var vasync = require('vasync'); var verror = require('verror'); var compose = require('restify').helpers.compose; var bunyan = require('bunyan'); + +// local globals var LOG; + /** * Install the enroute routes into the restify server. * * @param {object} opts Options object. * @param {object} opts.enroute The parsed enroute configuration. + * @param {object} opts.log an optional logger * @param {object} opts.server The restify server. * @param {string} opts.basePath The basepath to resolve source files. * @param {function} cb The callback. @@ -22,14 +26,21 @@ var LOG; */ function install(opts, cb) { assert.object(opts, 'opts'); + assert.optionalObject(opts.log, 'opts.log'); assert.object(opts.enroute, 'opts.enroute'); assert.object(opts.server, 'opts.server'); assert.string(opts.basePath, 'opts.basePath'); - if (typeof opts.log !== 'undefined') { - LOG = opts.log.child({ component: 'enroute' }); + var log; + + if (opts.log) { + log = opts.log.child({ component : 'enroute' }); } else { - LOG = bunyan.createLogger({name: 'enroute'}); + // only create default logger if one wasn't passed in. + if (!LOG) { + LOG = bunyan.createLogger({ name: 'enroute' }); + } + log = LOG; } vasync.pipeline({arg: {}, funcs: [ @@ -43,10 +54,10 @@ function install(opts, cb) { if (opts.enroute.hotReload) { if (typeof opts.basePath !== 'undefined') { - LOG.info({basedir: opts.basePath}, - 'Hot reloading of routes is enabled for base dir'); + log.info({ basedir: opts.basePath }, + 'hot reloading of routes is enabled for base dir'); } else { - LOG.info('Hot reloading of routes is enabled.'); + log.info('hot reloading of routes is enabled.'); } } @@ -60,25 +71,34 @@ function install(opts, cb) { var route; try { + var func = (opts.enroute.hotReload) ? + // restify middleware wrapper for hot reload + function enrouteHotReloadProxy(req, res, next) { + return reloadProxy({ + basePath: opts.basePath, + method: method, + req: req, + res: res, + routeName: routeName, + sourceFile: sourceFile + }, next); + } : require(sourceFile); + route = { name: routeName, method: method, - func: opts.enroute.hotReload - ? function (req, resp, callback) { - reloadProxy(sourceFile, req, - resp, callback, opts); - } - : require(sourceFile) + func: func }; } catch (e) { return cb1(new verror.VError({ + name: 'EnrouteRequireError', cause: e, info: { file: sourceFile, route: routeName, method: method } - }, 'route function is invalid')); + }, 'failed to require file, possible syntax error')); } // if HTTP method is 'delete', since restify uses 'del' // instead of 'delete', change it to 'del' @@ -112,7 +132,34 @@ function install(opts, cb) { }); } -function reloadProxy(sourceFile, req, resp, callback, opts) { + +/** + * using the restify handler composer, create a middleware on the fly that + * re-requires the file on every load executes it. + * @private + * @function reloadProxy + * @param {Object} opts an options object + * @param {String} opts.basePath The basepath to resolve source files. + * @param {String} opts.method http verb + * @param {Object} opts.log the enroute logger + * @param {Object} opts.req the request object + * @param {Object} opts.res the response object + * @param {String} opts.routeName the name of the restify route + * @param {String} opts.sourceFile the response object + * @param {Function} cb callback fn + * @returns {undefined} + */ +function reloadProxy(opts, cb) { + assert.object(opts, 'opts'); + assert.object(opts.basePath, 'opts.basePath'); + assert.string(opts.method, 'opts.method'); + assert.object(opts.log, 'opts.log'); + assert.object(opts.req, 'opts.req'); + assert.object(opts.res, 'opts.res'); + assert.string(opts.routeName, 'opts.routeName'); + assert.string(opts.sourceFile, 'opts.sourceFile'); + assert.func(cb, 'cb'); + if (typeof opts.basePath !== 'undefined') { //Delete code loaded from a specific base dir Object.keys(require.cache).forEach(function (cacheKey) { @@ -127,16 +174,30 @@ function reloadProxy(sourceFile, req, resp, callback, opts) { }); } - try { - var handlers = require(sourceFile); - var handlerChain = compose(handlers); + var handlers; - handlerChain(req, resp, callback); + try { + handlers = require(opts.sourceFile); } catch (e) { - LOG.error('Uncaught error in route:'); - LOG.error(e); - callback(e); + var err = new verror.VError({ + name: 'EnrouteRequireError', + cause: e, + info: { + file: opts.sourceFile, + route: opts.routeName, + method: opts.method + } + }, 'failed to require file, possible syntax error'); + + // now that the chain has failed, send back the require error. + return cb(err); } + + // if no require error, execute the handler chain. any errors that occur at + // runtime should be a runtime exception. + var handlerChain = compose(handlers); + return handlerChain(opts.req, opts.res, cb); } + module.exports = install; From 4432f27cc675fe1a12cacf307a499825423aae39 Mon Sep 17 00:00:00 2001 From: Alex Liu Date: Mon, 21 May 2018 19:44:27 -0700 Subject: [PATCH 3/6] fix: pr comments --- lib/install.js | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/lib/install.js b/lib/install.js index f2f79d1..bb099b1 100644 --- a/lib/install.js +++ b/lib/install.js @@ -53,12 +53,8 @@ function install(opts, cb) { }); if (opts.enroute.hotReload) { - if (typeof opts.basePath !== 'undefined') { - log.info({ basedir: opts.basePath }, + log.info({ basePath: opts.basePath }, 'hot reloading of routes is enabled for base dir'); - } else { - log.info('hot reloading of routes is enabled.'); - } } // go through each of the route names @@ -160,19 +156,12 @@ function reloadProxy(opts, cb) { assert.string(opts.sourceFile, 'opts.sourceFile'); assert.func(cb, 'cb'); - if (typeof opts.basePath !== 'undefined') { - //Delete code loaded from a specific base dir - Object.keys(require.cache).forEach(function (cacheKey) { - if (cacheKey.indexOf(opts.basePath) !== -1) { - delete require.cache[cacheKey]; - } - }); - } else { - //Delete all cached entries - Object.keys(require.cache).forEach(function (cacheKey) { + // clear require cache for code loaded from a specific base dir + Object.keys(require.cache).forEach(function (cacheKey) { + if (cacheKey.indexOf(opts.basePath) !== -1) { delete require.cache[cacheKey]; - }); - } + } + }); var handlers; From fb742d53f4680aafac6556fd46b6c6398a8709fe Mon Sep 17 00:00:00 2001 From: Yunong Xiao Date: Wed, 23 May 2018 15:11:12 -0700 Subject: [PATCH 4/6] add unit test for hot reloading --- README.md | 11 +++- lib/index.js | 4 +- lib/install.js | 9 ++-- package.json | 2 + test/etc/fooHotReload.js | 9 ++++ test/install.js | 108 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 136 insertions(+), 7 deletions(-) create mode 100644 test/etc/fooHotReload.js diff --git a/README.md b/README.md index 77774de..6e31c50 100644 --- a/README.md +++ b/README.md @@ -68,14 +68,21 @@ when done. * `opts`: The options object containing * `opts.server` The restify server to install the routes on to. * `[opts.config]` The POJO of the enroute config. - * `[opts.basePath]` Used with `[opts.config]`. The POJO requires a - `basePath` to correctly resolve the route source file(s). + * `[opts.basePath]` Used with `[opts.config]`. The POJO requires a + `basePath` to correctly resolve the route source file(s). * `[opts.configPath]` The path to the enroute config on disk. + * `[opts.hotReload]` Indicate whether you want the server to reload the + route from disk each time a request is served, + defaults to false * `cb` The callback. Returns `Error` if there's an error installing the routes. Note only one of `opts.config` or `opts.configPath` is needed. The module will either read in the file from disk, or use a pre-populated POJO. +`opts.hotReload` allows the restify server to reload the route from disk each +time the request is processed. This is extremely slow and should only be used +in non-production instances. + ### Example ```javascript const enroute = require('restify-enroute'); diff --git a/lib/index.js b/lib/index.js index 67c3b2b..71de598 100644 --- a/lib/index.js +++ b/lib/index.js @@ -18,6 +18,7 @@ module.exports = { * validate. * @param {string} [opts.configPath] The path to the data on disk to * validate. + * @param {boolean} [opts.hotReload] Whether to hot reload the routes * @param {string} opts.server The restify server to install the routes * onto. * @param {function} cb The callback f(err, result) @@ -39,7 +40,8 @@ module.exports = { install({ enroute: ctx.config, server: opts.server, - basePath: ctx.basePath + basePath: ctx.basePath, + hotReload: opts.hotReload }, function (err) { return _cb(err); }); diff --git a/lib/install.js b/lib/install.js index bb099b1..296f710 100644 --- a/lib/install.js +++ b/lib/install.js @@ -19,6 +19,7 @@ var LOG; * @param {object} opts Options object. * @param {object} opts.enroute The parsed enroute configuration. * @param {object} opts.log an optional logger + * @param {boolean} [opts.hotReload] Whether to hot reload the routes * @param {object} opts.server The restify server. * @param {string} opts.basePath The basepath to resolve source files. * @param {function} cb The callback. @@ -30,6 +31,7 @@ function install(opts, cb) { assert.object(opts.enroute, 'opts.enroute'); assert.object(opts.server, 'opts.server'); assert.string(opts.basePath, 'opts.basePath'); + assert.optionalBool(opts.hotReload, 'opts.hotReload'); var log; @@ -52,7 +54,7 @@ function install(opts, cb) { return cb1(); }); - if (opts.enroute.hotReload) { + if (opts.hotReload) { log.info({ basePath: opts.basePath }, 'hot reloading of routes is enabled for base dir'); } @@ -67,7 +69,7 @@ function install(opts, cb) { var route; try { - var func = (opts.enroute.hotReload) ? + var func = (opts.hotReload) ? // restify middleware wrapper for hot reload function enrouteHotReloadProxy(req, res, next) { return reloadProxy({ @@ -147,9 +149,8 @@ function install(opts, cb) { */ function reloadProxy(opts, cb) { assert.object(opts, 'opts'); - assert.object(opts.basePath, 'opts.basePath'); + assert.string(opts.basePath, 'opts.basePath'); assert.string(opts.method, 'opts.method'); - assert.object(opts.log, 'opts.log'); assert.object(opts.req, 'opts.req'); assert.object(opts.res, 'opts.res'); assert.string(opts.routeName, 'opts.routeName'); diff --git a/package.json b/package.json index 3a36987..e2a82b5 100644 --- a/package.json +++ b/package.json @@ -33,8 +33,10 @@ "chai": "^3.5.0", "coveralls": "^2.11.14", "eslint": "^3.8.1", + "fs-extra": "^6.0.1", "istanbul": "^0.4.5", "jscs": "^3.0.7", + "mkdirp": "^0.5.1", "mocha": "^3.1.2", "nsp": "^2.6.2", "restify-clients": "^1.4.0", diff --git a/test/etc/fooHotReload.js b/test/etc/fooHotReload.js new file mode 100644 index 0000000..8554960 --- /dev/null +++ b/test/etc/fooHotReload.js @@ -0,0 +1,9 @@ +'use strict' + +module.exports = function fooGet(req, res, next) { + res.header('name', 'foo'); + res.header('method', 'get'); + res.header('reload', 'yes'); + res.send(200); + next(); +}; diff --git a/test/install.js b/test/install.js index 791b29d..faa7476 100644 --- a/test/install.js +++ b/test/install.js @@ -4,15 +4,19 @@ var path = require('path'); var _ = require('lodash'); var assert = require('chai').assert; +var fsExtra = require('fs-extra'); +var mkdirp = require('mkdirp'); var restify = require('restify'); var restifyClients = require('restify-clients'); var vasync = require('vasync'); +var uuid = require('uuid'); var enroute = require('../lib/index'); var HOST = 'localhost' || process.env.HOST; var PORT = 1337 || process.env.PORT; var BASEPATH = path.join(__dirname, '..'); +var HOT_RELOAD_TMP_DIR = '/tmp/' + uuid.v4(); var CONFIG = { schemaVersion: 1, @@ -71,6 +75,64 @@ var CONFIG = { } }; +var HOT_RELOAD_CONFIG = { + schemaVersion: 1, + routes: { + foo: { + get: { + source: './fooGet.js' + }, + post: { + source: './fooPost.js' + }, + put: { + source: './fooPut.js' + }, + delete: { + source: './fooDelete.js' + }, + head: { + source: './fooHead.js' + }, + patch: { + source: './fooPatch.js' + }, + options: { + source: './fooOptions.js' + } + }, + bar: { + get: { + source: './barGet.js' + }, + post: { + source: './barPost.js' + }, + put: { + source: './barPut.js' + }, + delete: { + source: './barDelete.js' + }, + head: { + source: './barHead.js' + }, + patch: { + source: './barPatch.js' + }, + options: { + source: './barOptions.js' + } + }, + array: { + get: { + source: './arrayGet.js' + } + } + } +}; + + var SERVER; describe('enroute-install', function () { @@ -145,6 +207,52 @@ describe('enroute-install', function () { }); }); +describe('hot reload', function () { + before(function (done) { + SERVER = restify.createServer(); + /* eslint-disable consistent-return */ + mkdirp(HOT_RELOAD_TMP_DIR, function (err) { + if (err) { + return done(err); + } + // copy over the hot reloaded route + fsExtra.copy(BASEPATH + '/test/etc', + HOT_RELOAD_TMP_DIR, function (e2) { + return done(e2); + }); + }); + }); + + it('should hot reload routes', function (done) { + enroute.install({ + config: HOT_RELOAD_CONFIG, + server: SERVER, + basePath: HOT_RELOAD_TMP_DIR, + hotReload: true + }, function (err) { + assert.ifError(err); + // assert the routes were installed correctly + assertServer({}, function (err2) { + assert.ifError(err2); + // now we change fooGet.js to return a 'reload' header + fsExtra.copy(HOT_RELOAD_TMP_DIR + '/fooHotReload.js', + HOT_RELOAD_TMP_DIR + '/fooGet.js', function (err3) { + + assert.ifError(err3); + var client = restifyClients.createStringClient('http://' + + HOST + ':' + PORT); + client.get('/foo', function (err4, req, res, obj) { + assert.ifError(err4); + assert.equal('yes', res.headers.reload); + return done(); + }); + }); + }); + }); + }); + +}); + /// Privates From c410b4e8cb5f73b87d995edf2a17ce1883deae66 Mon Sep 17 00:00:00 2001 From: Yunong Xiao Date: Wed, 23 May 2018 15:30:03 -0700 Subject: [PATCH 5/6] drop suport for node 4, add support for 8, 10 --- .travis.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index f47a9ec..bad709c 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,8 +1,9 @@ language: node_js sudo: required node_js: - - "4" - "6" + - "8" + - "10" before_script: npm install -g yarn script: make all after_success: From 2037075fb8832391de77f1c43f7cf2a93b4f1b42 Mon Sep 17 00:00:00 2001 From: Yunong Xiao Date: Wed, 23 May 2018 15:34:35 -0700 Subject: [PATCH 6/6] dont add v10 support --- .travis.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index bad709c..6eb36c7 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,7 +3,6 @@ sudo: required node_js: - "6" - "8" - - "10" before_script: npm install -g yarn script: make all after_success: