Promises rejected without a value don't trigger failures

[jeffposnick]

(Moving details over from gulpjs/gulp#1642 (comment))

This is due to this code:

function onError(error) {
  return done(error);
}

If error is undefined, then this is equivalent to called done(undefined), which will be treated as a success, when it should really be a failure.

You can reproduce this via:

// Inside gulpfile.js:
const gulp = require('gulp');

gulp.task('reject-without-value', () => Promise.reject());
gulp.task('reject-with-value', () => Promise.reject('rejection reason'));

$ gulp -v
[14:52:22] CLI version 1.3.0
[14:52:22] Local version 4.0.0-alpha.2

$ gulp reject-without-value
[14:52:30] Using gulpfile /private/tmp/b/gulpfile.js
[14:52:30] Starting 'reject-without-value'...
[14:52:30] Finished 'reject-without-value' after 2.16 ms
$ echo $?
0

$ gulp reject-with-value
[14:52:37] Using gulpfile /private/tmp/b/gulpfile.js
[14:52:37] Starting 'reject-with-value'...
[14:52:37] 'reject-with-value' errored after 1.85 ms
[14:52:37] Error: rejection reason
    at formatError (/Users/jeffy/homebrew/lib/node_modules/gulp-cli/lib/versioned/^4.0.0/formatError.js:20:10)
    at Gulp.<anonymous> (/Users/jeffy/homebrew/lib/node_modules/gulp-cli/lib/versioned/^4.0.0/log/events.js:30:15)
    at emitOne (events.js:120:20)
    at Gulp.emit (events.js:210:7)
    at Object.error (/private/tmp/b/node_modules/undertaker/lib/helpers/createExtensions.js:61:10)
    at handler (/private/tmp/b/node_modules/now-and-later/lib/map.js:46:14)
    at f (/private/tmp/b/node_modules/once/once.js:25:25)
    at f (/private/tmp/b/node_modules/once/once.js:25:25)
    at done (/private/tmp/b/node_modules/async-done/index.js:24:15)
    at onError (/private/tmp/b/node_modules/async-done/index.js:32:12)
$ echo $?
1

[phated]

@jeffposnick I'm not sure the best way to resolve this. Do you think we should reject with an empty error object? I'm also nervous the stack trace will be incorrect due to us generating an object within our function.

[jeffposnick]

I'm not in a great position to say what the best practice would be. I'd imagine that I'd go with new Error(), which would lead to a stack trace that would look like it originated one level deeper than it should be, but would definitely be better than treating Promise.reject() as a success.

[phated]

@jeffposnick All set and released as 1.2.3

[phated]

Thanks for reporting this!!!