calls back twice on error

[ianstormtaylor]

just noticed that in the case of errors the callback function might actually be called twice the way it's currently done: https://github.com/visionmedia/consolidate.js/blob/master/lib/consolidate.js#L498-L500

is that a bug you think? or am i doing something rull dumb

[ForbesLindesay]

I submitted a pull request to fix this very issue (amongst a few others) about a year ago: #85

[tj]

it's both a bug and not a bug, throwing isn't great either, even node core has this issue but IMO it's a fundamental fail with callbacks

[ianstormtaylor]

would it be solved by this (or am i missing something rull dumb):

exports.handlebars.render = function(str, options, fn) {
  var engine = requires.handlebars || (requires.handlebars = require('handlebars'));
  try {
    for (var partial in options.partials) {
      engine.registerPartial(partial, options.partials[partial]);
    }
    for (var helper in options.helpers) {
      engine.registerHelper(helper, options.helpers[helper]);
    }
    var tmpl = cache(options) || cache(options, engine.compile(str, options));
    var ret = tmpl(options);
  } catch (err) {
    return fn(err);
  }
  fn(null, ret);
}

[tj]

kinda, fixes the double callback but then you'll just get uncaughts, which is worse is hard to say :D haha both suck, but node core will just let it propagate/uncaught so I suppose we might as well

[ForbesLindesay]

@ianstormtaylor Yes, it fixes the bug, but that change needs to be made to every synchronous engine's implementation, and there are other closely related bugs with many of the synchronous and asynchronous engines. e.g. the call to require should actually be inside the try/catch block.

@visionmedia This is correct behaviour - when you throw an un-handled exception, the app crashes. What we have currently is incorrect behaviour - when you throw an un-handled exception, a function gets unexpectedly called, either crashing your app or leaving it in an unexpected/undefined state. Promises (or shallow co-routines via ES6) make it easier to prevent/manage the un-handled exceptions if you are unhappy with the way callbacks work.

[tj]

well they're arguably both wrong, but node itself is wrong. I'm not saying double callbacks are good, but they're no more harmful than exploding the entire process from a template, they're both terrible ways to handle errors, both cases are user errors that need to be fixed

[ForbesLindesay]

How is the proposed solution wrong? Ideally you would use a system like promises to support error propagation, but not doing so doesn't seem "wrong", just not ideal.

[lennym]

Apologies for necromancing this issue, but I've just come up against it myself, and would definitely say it is a bug because while errors should be handled, this makes the handling of errors deeply unpredictable and counterintuitive. For example:

consolidate.ejs.render('...', {}, function (e, content) {
    if (e) {
        // the error thrown below ends up being handled here
    }
    throw new Error('error');
});

Furthermore, genuine code errors which should be left unhandled in development will potentially be incorrectly reported as failures in the template rendering, making issues difficult to debug.