added hot reload functionality #13
Conversation
lib/install.js
Outdated
| try { | ||
| route = { | ||
| name: routeName, | ||
| method: method, | ||
| func: require(sourceFile) | ||
| func: process.env.HOT_RELOAD === 'true' |
There was a problem hiding this comment.
shouldn't we turn this to an enroute option?
There was a problem hiding this comment.
Does that mean something in the opts object?
Can those be set from env vars so that we can enable it from tooling?
There was a problem hiding this comment.
Yes, we need to pick up env vars from NQ and pass enroute options.
But for the experiment, it's okay to leave it like this.
lib/install.js
Outdated
| if(typeof process.env.HOT_RELOAD_BASEDIR !== 'undefined') { | ||
| //Delete code loaded from a specific base dir | ||
| Object.keys(require.cache).forEach(function (k) { | ||
| if(k.indexOf(process.env.HOT_RELOAD_BASEDIR) !== -1) { |
There was a problem hiding this comment.
k.includes(process.env.HOT_RELOAD_BASEDIR) also k -> moduleName (or what it is)?
There was a problem hiding this comment.
k is the cache key. This is whatever require resolved to, which is usually a file path.
Rename to cacheKey maybe?
lib/install.js
Outdated
| @@ -6,6 +6,7 @@ var _ = require('lodash'); | |||
| var assert = require('assert-plus'); | |||
| var vasync = require('vasync'); | |||
| var verror = require('verror'); | |||
| var chainComposer = require('restify').helpers.chainComposer; | |||
There was a problem hiding this comment.
just require('restify').helpers.compose
package.json
Outdated
| "restify-clients": "^1.4.0", | ||
| "uuid": "^2.0.3" | ||
| }, | ||
| "dependencies": { | ||
| "ajv": "^4.8.0", | ||
| "assert-plus": "^1.0.0", | ||
| "lodash": "^4.16.4", | ||
| "restify": "^7.0.0", |
|
@hekike Updated this PR to use bunyan for logging and clean up the code. |
lib/install.js
Outdated
| @@ -32,6 +37,15 @@ function install(opts, cb) { | |||
| return cb1(); | |||
| }); | |||
|
|
|||
| if (process.env.HOT_RELOAD) { | |||
There was a problem hiding this comment.
@DonutEspresso shouldn't this be an option and passing this ENV (property) in NQ?
There was a problem hiding this comment.
Yes +1 - let's make this a first class option and we can thread through the env var from a lower level.
lib/install.js
Outdated
| @@ -6,6 +6,11 @@ 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 = bunyan.createLogger({name: 'enroute'}); | |||
There was a problem hiding this comment.
Rather than create a new logger we should have enroute here create a child logger from an incoming logger - we'll make sure NQ threads it through:
var log = opts.log.child({ component: 'enroute' });There was a problem hiding this comment.
Doesn't that require another Restify release first, because today it doesn't pass a logger?
lib/install.js
Outdated
| @@ -32,6 +37,15 @@ function install(opts, cb) { | |||
| return cb1(); | |||
| }); | |||
|
|
|||
| if (process.env.HOT_RELOAD) { | |||
| if (typeof process.env.HOT_RELOAD_BASEDIR !== 'undefined') { | |||
There was a problem hiding this comment.
If there are a multiple options for the hot reload option we should just make opts.hotReload a top level option.
| "lodash": "^4.16.4", | ||
| "restify": "^7.2.0", |
There was a problem hiding this comment.
Once we use child loggers I don't think we'll need bunyan anymore. What's restify needed for? Or is that really a devDependency?
There was a problem hiding this comment.
Restify exports the new helpers.compose function to create a handler chain, which is used in the reloadProxy function.
renaming variable for readability extra logging Flatten handlers array using composer function to build handler chain now using a released version of restify using bunyan for logging and cleaned up code using log context for logs hotReload is now an enroute option Using child logger if set as option
|
@DonutEspresso Now using child logger if a logger was passed in options and hotReload is option as well. |
|
Thanks @paulbakker ! I'm going to merge this into an origin branch and carry it through the finish line - appreciate the help! |
| @@ -32,6 +41,15 @@ function install(opts, cb) { | |||
| return cb1(); | |||
| }); | |||
|
|
|||
| if (opts.enroute.hotReload) { | |||
| if (typeof opts.basePath !== 'undefined') { | |||
There was a problem hiding this comment.
@paulbakker one qq - in what cases was basePath not defined?
There was a problem hiding this comment.
It was an assumption that it’s possibly not defined. If this option is always required we can go without the check.
There was a problem hiding this comment.
Cool thanks! It's always required, so I think we can simply the check. 👍
Currently importing
restify/lib/chain. This will be fixed before releasing this as discussed in person with @hekike.