Conversation
doowb
left a comment
There was a problem hiding this comment.
Thanks. I've added a few change requests, but I think this looks good!
| return promisify(cb, function(cb) { | ||
| readPartials(path, options, function(err) { | ||
| readPartials(path, options, function(err, partials) { | ||
| var extend = (requires.extend || (requires.extend = require('util')._extend)); |
There was a problem hiding this comment.
I don't think this is necessary, you can just use Object.assign.
There was a problem hiding this comment.
you sure? I wasn't sure which version needs to be supported since only extend is used in this lib
There was a problem hiding this comment.
thanks for pointing that out... I forgot that it was already used in other places.
lib/consolidate.js
Outdated
| } | ||
| var tmpl = cache(options) || cache(options, engine.template(str, null, options)); | ||
| cb(null, tmpl(options).replace(/\n$/, '')); | ||
| const optionsCopy = extend({}, options); |
There was a problem hiding this comment.
I don't think making a copy of options is necessary here since the copy was already made in fromStringRenderer above and already has the new partials object.
lib/consolidate.js
Outdated
| readPartials(path, options, function(err) { | ||
| readPartials(path, options, function(err, partials) { | ||
| var extend = (requires.extend || (requires.extend = require('util')._extend)); | ||
| var optionsCopy = extend({}, options); |
There was a problem hiding this comment.
just a nitpick... instead of optionsCopy, change this to opts.
|
@marek-nogiec thanks for the PR! I'll try to get this published soon. |
Hi, this is related to #302
I took the approach suggested in that issue (returning new
partialsin the cb ofreadPartials) and merging that with copiedoptions.let me know what you think