gulpjs / undertaker

Task registry that allows composition through series/parallel methods.
MIT License
200 stars 31 forks source link

Error handling #66

Closed Reinmar closed 7 years ago

Reinmar commented 7 years ago

I've been using Undertaker in the following way:

function uberTask() {
    return new Promise( ( resolve ) => {
        const taker = new Undertaker();

        taker.task( 'a', () => {
        } );

        taker.task( 'b', () => {
        } );

       taker.series( 'a', 'b', resolve )();
    } );
}

If any of the tasks throws, then I get some useless stack on the console:

[10:41:25] Using gulpfile /www/ckeditor5/ckeditor5/gulpfile.js
[10:41:25] Starting 'test'...
events.js:165
      throw err;
      ^

Error: Uncaught, unspecified "error" event. ([object Object])
    at Undertaker.emit (events.js:163:17)
    at Object.error (/www/ckeditor5/ckeditor5/node_modules/@ckeditor/ckeditor5-dev-tests/node_modules/undertaker/lib/helpers/createExtensions.js:61:10)
    at handler (/www/ckeditor5/ckeditor5/node_modules/@ckeditor/ckeditor5-dev-tests/node_modules/now-and-later/lib/mapSeries.js:43:14)
    at f (/www/ckeditor5/ckeditor5/node_modules/@ckeditor/ckeditor5-dev-tests/node_modules/once/once.js:25:25)
    at f (/www/ckeditor5/ckeditor5/node_modules/@ckeditor/ckeditor5-dev-tests/node_modules/once/once.js:25:25)
    at done (/www/ckeditor5/ckeditor5/node_modules/@ckeditor/ckeditor5-dev-tests/node_modules/async-done/index.js:24:15)
    at Domain.onError (/www/ckeditor5/ckeditor5/node_modules/@ckeditor/ckeditor5-dev-tests/node_modules/async-done/index.js:32:12)
    at Domain.g (events.js:286:16)
    at emitOne (events.js:96:13)
    at Domain.emit (events.js:188:7)
    at Domain.errorHandler [as _errorHandler] (domain.js:97:23)
    at process._fatalException (node.js:265:33)

I guess that I should handle the errors in this way:

return new Promise( ( resolve, reject ) => {
    const taker = new Undertaker();

    taker.task( 'a', () => {
        throw new Error( 'a' );
    } );

    taker.task( 'b', () => {
    } );

    taker.on( 'error', ( error ) => {
        reject( error.error );
    } );

    taker.series( 'a', 'b', resolve )();
} );

Am I right? I haven't found this in the documentation.

phated commented 7 years ago

You are overthinking this. It is not recommended to call the method returned by taker.series directly, the wrapping libraries take care of that for you.

You shouldn't be wrapping taker.task or taker.series in a promise.

If you need to register custom tasks, use a Custom Registry.

If you are just using custom functions for taker.series, you can just pass it functions directly to the method; you don't have to register them as tasks first.

You most definitely should not be listening to the events emitted by undertaker. They are undocumented and likely to change. They are an implementation detail that leaked to wire up gulp-cli and task resolution.

Reinmar commented 7 years ago

Thanks for clarifying this.

I must say I'm a bit surprised. I thought that Undertaker will be a great tool to do a gulp-like task management, but outside of gulp.

I'm writing a library which is meant to expose some tasks, but which also should be reusable and easy to use. Hence, it exposes low level tasks (e.g. compileFoo(), compileBar()) and some higher level ones (e.g. compileAll() which internally will call the compileFoo() and compileBar() ones). However, I can't use gulp for that because:

So, for me tasks like compileFoo() are just functions which can return stream, promise or nothing. In order to implement compileAll() I need to sync all of them together and this is where I thought I can use undertaker.

It seemed a perfect solution and now I'm puzzled, cause I don't understand:

And, of course, I'd like to know how to solve my case ;). Unfortunately, as you can see, I found the documentation confusing.

The funny thing is that the way I use it, it solves my problems perfectly. For me, this proves undertaker's value and I'd be super happy if its scope could be extended to also cover use cases like mine.

phated commented 7 years ago

If you'd like to open another issue about this use case with more in-depth examples, I'd be interested in discussing it further and helping you solve/understand the usage. Your usage sounds interesting and I'm quite intrigued.

phated commented 7 years ago

@Reinmar are you still interested in opening another issue to discuss your use case further?

Reinmar commented 7 years ago

Yep, I've just opened https://github.com/gulpjs/undertaker/issues/67.