Improve error messages on exit

[pkozlowski-opensource]

I wonder how we could improve error messages when a task signals an error in a callback. Consider this convoluted (but illustrating the point) task:

gulp.task('foo', function (done) {
  setTimeout(function () {
    done(new Error('fail'));
  }, 1000);
});

With the above code snippet the exit code / stack trace is nice and all is well:

[10:46:52] 'foo' errored after 1 s
[10:46:52] Error: fail
at null._onTimeout (/Users/pawelkozlowski/work/gitrepos/pkozlowski-opensource/gulp-karma/gulpfile.js:34:10)
at Timer.listOnTimeout as ontimeout

The problem

I supply the done callback with a string (done('fail')) I'm getting a looongish stack trace from the gulp CLI which is rather verbose and hard to read:

[10:51:47] Error: fail
at formatError (/usr/local/lib/node_modules/gulp/bin/gulp.js:152:12)
at Gulp. (/usr/local/lib/node_modules/gulp/bin/gulp.js:189:15)
at Gulp.EventEmitter.emit (events.js:95:17)
at Gulp.Orchestrator._emitTaskDone (/Users/pawelkozlowski/work/gitrepos/pkozlowski-opensource/gulp-karma/node_modules/gulp/node_modules/orchestrator/index.js:264:8)
at /Users/pawelkozlowski/work/gitrepos/pkozlowski-opensource/gulp-karma/node_modules/gulp/node_modules/orchestrator/index.js:275:23
at finish (/Users/pawelkozlowski/work/gitrepos/pkozlowski-opensource/gulp-karma/node_modules/gulp/node_modules/orchestrator/lib/runTask.js:20:8)
at cb (/Users/pawelkozlowski/work/gitrepos/pkozlowski-opensource/gulp-karma/node_modules/gulp/node_modules/orchestrator/lib/runTask.js:28:3)
at null._onTimeout (/Users/pawelkozlowski/work/gitrepos/pkozlowski-opensource/gulp-karma/gulpfile.js:34:5)
at Timer.listOnTimeout as ontimeout

It has all the necessary info but yeh, people will be probably confused by seeing orchestrator / gulp as part of the stack-trace. Maybe it would make sense to filter out part of the internal Gulp stack-frames?

Special case for numbers

If a non-zero number is supplied to the done callback (ex. done(1)), which is a real life scenario from gulp-karma integration (https://github.com/karma-runner/gulp-karma/blob/master/gulpfile.js#L20) we are getting:

[10:59:14] 'foo' errored after 1 s
[10:59:14] undefined

which should be fixed, IMO, as undefined looks like something is internally broken.

The proposal

Based on the above, here are my suggestions:

• let's treat number exit values the same way as string exit values (or, more generally - all the non-Error exit values should be going through the same path);
• consider filtering out parts of the stack-trace to remove Gulp / orchestrator frames. It would be really nice for users but yeh, this is rather fragile, so I'm 50/50 here.

Would be happy to put together a PR if the above reasoning makes sense to you guys.

Related to this commit: aeaebd9

[yocontra]

Stacktraces will be handled with the BetterError module I'm working on right now, which should make them look a lot better and allow for easier filtering + visualization of async operations.

Why pass a number to done instead of just passing an error?

[pkozlowski-opensource]

Stacktraces will be handled with the BetterError module I'm working on right now, which should make them look a lot better and allow for easier filtering + visualization of async operations.

Awesome! If you've got any WIP / design docs to share I would definitively have a look.

Why pass a number to done instead of just passing an error?

This is due to the fact that some 3rd party libraries are taking callbacks to signal their async processing completion. In this particular case I was bridging Karma-runner with gulp (full repo here: https://github.com/karma-runner/gulp-karma):

var karma = require('karma').server;

gulp.task('test', function (done) {
  karma.start({}, done);
});

As you can see I'm directly passing gulp's done callback down to Karma and it is Karma that calls back with the status 1. Of course I could do:

gulp.task('test', function (done) {
  karma.start({}, function(exitStatus){
    done(exitStatus ? "There are failing unit tests" : undefined);
  });
});

but yeh, this is more verbose and not really needed as Karma already provides good info about the run status / failed tests.

So, this is real-life scenario and IMO it wouldn't hurt to handle strings and numbers the same way.

[yocontra]

Can you test out the code on master and see if it works for you? The stack traces are still fucked but that's being handled in gulp 4

[pkozlowski-opensource]

Yup, this seems consistent now, thnx @contra