set errors on the chunk when catching errors so that the proper callbacks are invoked... #471

prashn64 · 2014-06-05T03:26:38Z

when rendering or streaming

…acks are always invoked when rendering or streaming

jimmyhchan · 2014-06-05T14:16:05Z

Travis failed?

cpettitt · 2014-06-05T14:28:27Z

I see this on my unrelated node repos too. It seems that some common node dependency (minimatch) is no longer available in node 0.8.

cpettitt · 2014-06-05T14:34:01Z

More details on the minimatch problem: isaacs/minimatch#35. The following .travis.yml snippet from the linked issue fixed it for one of my projects:

before_install: npm update -g npm

cpettitt · 2014-06-05T16:13:39Z

I confirmed this fixes the immediate failures I'm seeing. However, this still leaves open the possibility of losing failures in onLoad. Here's an example:

<!doctype html>

<script src="dust-full.js"></script>
<script>
  dust.onLoad = function(name, callback) {
    dust.nextTick(function() {
      switch (name) {
        case 'bad': callback(null, "BEFORE_BAD {@bad/} AFTER_BAD"); break;
        default: callback('No such template!'); break;
      }
    });
  };

  dust.helpers.bad = function() {
    throw new Error("Ooops!");
  };

  var stream;
  try {
    stream = dust.stream("bad", {});
  } catch (e) {
    console.log("DUST STREAM THREW", e);
  }

  stream.on("data", function(data) {
    console.log("STREAM DATA", data);
  });
  stream.on("error", function(err) {
    console.log("STREAM ERROR", err);
  });
  stream.on("end", function() {
    console.log("STREAM END");
  });
</script>

The chrome console shows an empty chunk followed by an uncaught error:

STREAM DATA  index.html:26
Uncaught Error: Ooops! index.html:15
dust.helpers.bad index.html:15
Chunk.helper dust-full.js:787
body_0 VM335:1
(anonymous function) dust-full.js:156
(anonymous function)

Still, this change is a big improvement over the current state. I'd be happy to have it. We may just have to disable lazy template loading until the larger issue is resolved.

jimmyhchan · 2014-06-05T16:20:58Z

Looks like our dev dependency grunt-template-jasmine-istanbul uses fileset which specifies only 0.X for minimatch https://github.com/mklabs/node-fileset.

0.8 will be all but lost if Grunt moves to minimatch 0.3

jimmyhchan · 2014-06-06T18:09:01Z

test/jasmine-test/spec/coreTests.js

@@ -1829,19 +1829,6 @@ var coreTests = [
 message: "test the log messages for an existing not exists check."
 },
 {
- name: "Errors should be propogated if the silenceErrors flag is not set",


this is not backwards compatible. 2.4?

Errors are still propagated, but in the way that you can actually catch them :). I don't mind bumping to 2.4 though.

With this change do we still need dust.silenceErrors flag?

+1 deprecate silenceErrors and deprecate dust.log throwing errors.

Check the new dust.throwError. It's still useful if you want to continue rendering in certain cases when throwing an error.

throwError is a misnomer.... maybe dust.fail instead? If so, it should fail pretty catastrophically.

or... chunk.fail so that it's scoped to within the helper user function code.

as discussed there isn't really a use case for trying to throw errors since they are meaningless for the async cases.

for consistency, if you want to throw use chunk.setError and if you want to catch look in the errorback

…he same function, remove error throwing from dust.log, and set a chunk error when dust helper errors are caught

jimmyhchan · 2014-06-06T23:36:23Z

This looks good for a 2.4 release (since we deprecated so much stuff). We should do something for those who want to throw real errors (see #381) for example, chunk.fail.

kate2753 · 2014-06-10T21:39:51Z

Looks good to me.

What is the use case for throwing real errors? Keep in mind that both dust.render and dust.stream are asynchronous and wrapping those calls in try\catch won't catch real errors thrown.

smfoote · 2014-06-10T22:03:23Z

Looks good to me, too.

On Tue, Jun 10, 2014 at 2:39 PM, Kate Odnous notifications@github.com
wrote:

Looks good to me.

What is the use case for throwing real errors? Keep in mind that both
dust.render and dust.stream are asynchronous and wrapping those calls in
try\catch won't catch real errors thrown.

—
Reply to this email directly or view it on GitHub
#471 (comment).

Errors thrown from render will now call populate the error in the callback. Previously, thrown errors will immediately fail and not call the callback. Issue #381, Address #468: Errors thrown from stream now will invoke the `error` listener with the error object. Previously, these errors could not be caught and the process will hang. Deprecated/remove: `dust.onError`, `dust.silenceErrors`. To see runtime errors, look at the error in the callback. breaking change: `dust.log` with an error no longer throws that error

cpettitt · 2014-06-17T15:25:24Z

Did we do anything to address the asynchronous load problem mentioned in #471 (comment)? If not, should I open a separate ticket for that?

prashn64 · 2014-06-17T23:54:40Z

Yeah, let's open up a new ticket.

set errors on the chunk when catching errors so that the proper callb…

d1b39eb

…acks are always invoked when rendering or streaming

prashn64 mentioned this pull request Jun 5, 2014

No notification on error in helper #468

Closed

jimmyhchan reviewed Jun 6, 2014
View reviewed changes

Prayrit Jain added 3 commits June 6, 2014 13:09

rename dust.onError to dust.throwError, stop returning a chunk from t…

3518bc2

…he same function, remove error throwing from dust.log, and set a chunk error when dust helper errors are caught

remove dust.throwError/dust.onError

0dea16b

remove dust.silenceErrors

2ff9ffe

jimmyhchan merged commit 12d8690 into linkedin:master Jun 11, 2014

cpettitt mentioned this pull request Jun 18, 2014

Dust does not propagate thrown errors from a helper if async onLoad is used #481

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set errors on the chunk when catching errors so that the proper callbacks are invoked... #471

set errors on the chunk when catching errors so that the proper callbacks are invoked... #471

prashn64 commented Jun 5, 2014

jimmyhchan commented Jun 5, 2014

cpettitt commented Jun 5, 2014

cpettitt commented Jun 5, 2014

cpettitt commented Jun 5, 2014

jimmyhchan commented Jun 5, 2014

jimmyhchan Jun 6, 2014

prashn64 Jun 6, 2014

kate2753 Jun 6, 2014

jimmyhchan Jun 6, 2014

prashn64 Jun 6, 2014

jimmyhchan Jun 6, 2014

jimmyhchan Jun 6, 2014

jimmyhchan Jun 11, 2014

jimmyhchan commented Jun 6, 2014

kate2753 commented Jun 10, 2014

smfoote commented Jun 10, 2014

cpettitt commented Jun 17, 2014

prashn64 commented Jun 17, 2014

set errors on the chunk when catching errors so that the proper callbacks are invoked... #471

set errors on the chunk when catching errors so that the proper callbacks are invoked... #471

Conversation

prashn64 commented Jun 5, 2014

jimmyhchan commented Jun 5, 2014

cpettitt commented Jun 5, 2014

cpettitt commented Jun 5, 2014

cpettitt commented Jun 5, 2014

jimmyhchan commented Jun 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmyhchan commented Jun 6, 2014

kate2753 commented Jun 10, 2014

smfoote commented Jun 10, 2014

cpettitt commented Jun 17, 2014

prashn64 commented Jun 17, 2014