Yunong/3.0 #4
Yunong/3.0 #4
Conversation
|
Changes Unknown when pulling 35149a8 on yunong/2.0 into * on master*. |
|
Changes Unknown when pulling 051e74a on yunong/2.0 into * on master*. |
|
Changes Unknown when pulling 051e74a on yunong/2.0 into * on master*. |
|
Changes Unknown when pulling 5eca780 on yunong/2.0 into * on master*. |
1 similar comment
|
Changes Unknown when pulling 5eca780 on yunong/2.0 into * on master*. |
| * `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.configPath` The path to the enroute config on disk. |
There was a problem hiding this comment.
Are config and configPath mutually exclusive?
There was a problem hiding this comment.
Yep, they are - would recommend documenting that here (and also add [] to indicate optionalness as you have in the JSDocs).
| * @param {function} cb The callback. | ||
| * @returns {undefined} | ||
| */ | ||
| function install(opts, cb) { |
There was a problem hiding this comment.
I've started this convention where the function level cb is always callback, and use cb, cb2 for nested async ops. It's nice because you see right away where you exit the top level fn.
| /* eslint-enable no-eval */ | ||
| }; | ||
| } catch (e) { | ||
| return _cb(e); |
There was a problem hiding this comment.
Might want to use VError or something to wrap some more contextual info? Like the file name and location, etc.
| } else { | ||
| fs.readFile(opts.configPath, 'utf8', function (err, config) { | ||
| if (err) { | ||
| _cb(new verror.VError( |
There was a problem hiding this comment.
You can use the info property and attach the file path, more context ftw.
| it.config = JSON.parse(config); | ||
| _cb(); | ||
| } catch (e) { | ||
| _cb(e); |
There was a problem hiding this comment.
Also probably want a VError here.
| var errors = _.map(validate.errors, function (fakeErr) { | ||
| return new verror.VError({ | ||
| name: 'EnrouteConfigCompileError', | ||
| info: fakeErr |
There was a problem hiding this comment.
ajv doesn't return real error objects?
| if (cb) { | ||
| return cb(err); | ||
| } else { | ||
| throw err; |
There was a problem hiding this comment.
In what scenario is this called without a callback?
There was a problem hiding this comment.
Nice catch, some vestigial code here -- i can clean up.
| "source": "./routes/getAcceptCookies.js" | ||
| } | ||
| "schemaVersion": 1, | ||
| "foo": { |
There was a problem hiding this comment.
If routes are top level, thoughts on If we add more metadata in the future?
There was a problem hiding this comment.
hmm, fair point, presumably we'd move to V2.
There was a problem hiding this comment.
Updated the PR -- added 'routes' as a top level property.
| }, | ||
| function validateInput(it, _cb) { | ||
| _validateInput(it.schema, it.config, function (err, res) { | ||
| result = res; |
There was a problem hiding this comment.
This result/res dance is kind of confusing. Would recommend renaming res since that's commonly used for response objects.
Why not just have the final errback at L:99 return the result of the last operation instead of using a closure variable?
There was a problem hiding this comment.
The naming is confusing. result is actually the parsed config, and not the vasync result object. I'll rename it.
| @@ -60,9 +59,7 @@ lint: node_modules $(ESLINT) $(SRCS) | |||
| # make nsp always pass - run this as separate travis task for "reporting" | |||
| .PHONY: nsp | |||
| nsp: node_modules $(NSP) | |||
| $(NPM) shrinkwrap --dev | |||
There was a problem hiding this comment.
If you remove the shrinkwrap it won't check for issues with transitive module (which is typically where most security issues occur, at lower level modules). Any reason why we don't want to have that visibility?
There was a problem hiding this comment.
Orrrr... is this covered by yarn now?
There was a problem hiding this comment.
Didn't realize it was needed by nsp, i can revert.
|
Changes Unknown when pulling 72fd7f5 on yunong/2.0 into * on master*. |
|
Changes Unknown when pulling c40558e on yunong/2.0 into * on master*. |
|
Changes Unknown when pulling 58b2bb3 on yunong/2.0 into * on master*. |
|
Changes Unknown when pulling 79cd761 on yunong/2.0 into * on master*. |
1 similar comment
|
Changes Unknown when pulling 79cd761 on yunong/2.0 into * on master*. |
|
Changes Unknown when pulling 47b6cfb on yunong/2.0 into * on master*. |
|
@DonutEspresso addressed PR concerns -- please take a look as I want to merge and publish. |
|
Changes Unknown when pulling 30dd69e on yunong/2.0 into * on master*. |
1 similar comment
|
Changes Unknown when pulling 30dd69e on yunong/2.0 into * on master*. |
|
@DonutEspresso going to merge for now -- feel free to fire off additional comments. |
yunong
dismissed
DonutEspresso’s stale review
Addressed CR comments, Alex is on vacation, he's free to add additional comments when he returns.
@DonutEspresso @rajatkumar PTAL
Cleanup and rewrite of enroute.