adamayres / gulp-wrap

A gulp plugin to wrap the stream contents with a lodash template.
https://www.npmjs.com/package/gulp-wrap
MIT License
84 stars 17 forks source link

Adds a try-catch block to wrap consolidate's render method to handle exceptions #21

Closed cheton closed 9 years ago

cheton commented 9 years ago

Some template render methods in the consolidate library were implemented like this:

// https://github.com/tj/consolidate.js/blob/master/lib/consolidate.js
exports.lodash.render = function(str, options, fn) {
  var engine = requires.lodash || (requires.lodash = require('lodash'));
  try {
    var tmpl = cache(options) || cache(options, engine.template(str, null, options));
    fn(null, tmpl(options).replace(/\n$/, ''));
  } catch (err) {
    fn(err);
  }
};

The require() method is not wrapped in the try-catch block. If the template engine is not installed, the require() method will throw an error immediately, and there is no chance to catch errors in render's fn callback.

I suggest adding a try-catch block to wrap consolidate's render method to handle exceptions, and provide an example of error handling in the documentation as the following:

var gulp = require("gulp");
var gutil = require("gulp-util");
var wrap = require("gulp-wrap");

gulp.src("./src/*.json")
    .pipe(wrap("<%= contents %>"))
        .on("error", gutil.log);
    .pipe(gulp.dest("./dist"));
shinnn commented 9 years ago

If the template engine is not installed, the require() method will throw an error immediately

This behavior looks fine for me. When the script fails to load the template engine, we should stop the process and rewrite the gulpfile. There is no need to handle such an error.

cheton commented 9 years ago

After I upgrade gulp-wrap to v0.10.x, if default template engine lodash is not installed correctly, there is no error message produced in the console and no output files are generated. It's difficult to troubleshoot because there is no error message.

Once I downgrade to v0.9.x, I'm able to see the error message and is aware of what is missing.

shinnn commented 9 years ago

I reproduced the problem and found a better solution.

I'll fix it soon. Thanks for reporting.

shinnn commented 9 years ago

Fixed in v0.11.0.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-1.5%) to 88.89% when pulling d63c1ac2d8e5f144aeb36894129d26f021602171 on cheton:master into 03fb42b7c69226d75041d7359248958f0dbb0688 on adamayres:master.

cheton commented 9 years ago

The solution looks good to me. Thanks for your prompt support.