Support for returning promises, and async/await functions.

balupton commented 8 years ago

Promises run immediately, and thus all at once, do not catch/isolate uncaught async errors, and can lose errors if .catch was not specified pedantically. Tasks run when you tell them too and using domains can catch/isolate uncaught errors within a task, and will throw errors if not handled. TaskGroup can thus control the concurrency, and be configured on how to deal with error situations - continue, pause, or by default abort and clear - and again, will throw errors if not handled. These factors alone provide why TaskGroup is superior to Promises, however, nevertheless, Promises are popular, and we should provide ways of supporting them to make integration, and perhaps conversion, with that scene easier.

The relevant layers of TaskGroup are:

ambi for async/sync method signature normalisation, used by Task, to wrap execute methods
Task and TaskGroup

Here are the options I can determine.

Consumption

Add promise consumption to ambi, such that one can do this:

// note the promise runs when the task runs
Task.create(function () {
    return new Promise(function(resolve, reject) {
        setTimeout(function () {
            const result = Math.floor(Math.random() * 1000)
                if ( result % 2 ) {
                    resolve(result)
            }
            else {
                reject(new Error('result did not become an even number'))
            }
        }, 1000)
    })
})

Add promise consumption support to Task, such that one can do this:

// note the promise runs immediately, and will not support things such as task arguments
Task.create(new Promise(function(resolve, reject) {
    setTimeout(function () {
        const result = Math.floor(Math.random() * 1000)
        if ( result % 2 ) {
            resolve(result)
        }
        else {
            reject(new Error('result did not become an even number'))
        }
    }, 1000)
})

Add promise consumption support to TaskGroup, such that one can do this:

// note the promise runs immediately, and will not support things such as task arguments
tasks.addPromise(new Promise(function(resolve, reject) {
    setTimeout(function () {
        const result = Math.floor(Math.random() * 1000)
        if ( result % 2 ) {
            resolve(result)
        }
        else {
            reject(new Error('result did not become an even number'))
        }
    }, 1000)
})

Production

Exposing promises could be done via providing a .promise getter instead of .done(callback):

// Task
Task.create(someTaskMethod).run().promise
    .then(console.log)
    .catch(console.error)

// TaskGroup
TaskGroup.create({storeResult: true}).addTasks(someTasks).run().promise
    .then(console.log)
    .catch(console.error)

I lean towards the above over having Task and TaskGroup extend the Promise class, as the promise API is an ever expanding glob or crap trying to workaround their inherent shortcomings - hence the introduction of their proposed done method and other official and non-official spec modifications implemented by the vast array of promise scene implementations.

However, the above requires the consumer to be aware that what they are consuming is a Task or TaskGroup, rather than just a promise, which if you are writing a simple library, may not be ideal. Once could write a wrapper around Task and TaskGroup, that could swap out their excellent API for the wrose Promise API. Such as:

// Include special versions
const {Task, TaskGroup} = require('taskgroup-promise')

// Task
Task.create((x, y) => x * y).run().promise
    .then(console.log)
    .catch(console.error)

// TaskGroup
TaskGroup.create({storeResult: true}).addTasks(someTasks).run().promise
    .then(console.log)
    .catch(console.error)

However, I am not sure fragmenting the TaskGroup ecosystem makes sense in this way, hence why I still lean towards a .promise getter.

Conclusions

Other ideas and discussion are welcome.

For consumption, seems adding promise consumption to ambi makes the most sense, as adding support to Task and TaskGroup means promises fire immediately, and there will be no support for things like task args. To make things easier, such that Task.create(new Promise(...)) is supported, ambi could check if the method is already a promise, before executing it and checking the return result. A question here, is what if a method returns a promise and accepts a callback as many interop libraries do.

For production, seems doing .promise is the best idea. Perhaps even with a getter for .then and .catch to alias .promise.then and .promise.catch for easier interop - however I am not sure if implementing such things will pass the promise scene's isPromise checks:

// note the promise runs when the task runs
Task.create(function (complete) {
    const p = new Promise(function(resolve, reject) {
        setTimeout(function () {
            const result = Math.floor(Math.random() * 1000)
                if ( result % 2 ) {
                    resolve(result)
            }
            else {
                reject(new Error('result did not become an even number'))
            }
        }, 1000)
    })
        if ( complete ) p.then((result) => complete(null, result)).catch(complete)
        return p  // could be `else return p` but that would not have any problem
})

Would be interesting to see how ambi or Task handles the above case, in terms of what error or enforcement it should produce, or whether it should discard the callback or the promise.

It is also worth mentioning our Chainy project, that provides the chaining abilities of promises to the TaskGroup ecosystem, in a microjs way.

balupton commented 7 years ago

So I think that TaskGroup and Task should function as promises in the upcoming release, as well as check for promises. Ambi should not be updated.

what will need to be done here is compliance with:

franciscolourenco commented 7 years ago

What changed your mind into this path?

balupton commented 7 years ago

Well, all functionality will be the same. The only changes it seems are having .then and .catch and perhaps the constructors behave according to the promise spec. As well as the execution code checking if a promise is returned. Which should be a fairly small change. A fairly small change that will allow interop with the promise workflow, async/await, etc - while keeping all the power and API niceness of TaskGroup/Tasks.

This approach wins out, over something like a .promise, as it makes the promise workflow easier to access. Anytime a TaskGroup or Task is used, a promise workflow becomes possible. It also means that Chainy has the potential to work with any promise library.

The reasoning for no changes to ambi, is that if TaskGroup/Task add promise interop directly, then ambi can serve merely has the async callback wrapper - as if people have a pure promise workflow, they may want to disable ambi completely - as there is no way ambi can operate without the risk of injecting a next callback, which a promise workflow may not desire. Otherwise the updates to ambi could be the following:

Check if the passed argument to ambi is a promise instance
Check if the last argument is a completion callback name (using something like thing.toString()) - however this fails for .binds() and obfuscation

That said, the power of ambi's function.length approach works incredibly well, as it is quite intuitive. But in cases were people desire to pass less arguments than a function expects, then a next callback would be provided as the last argument, even if that last argument was not intended as a callback - which seems common in promise workflows. So in those promise workflows, turning ambi off would be desired - turning ambi off while maintaining promise support is needed.

balupton commented 6 years ago

pushed up the initial work for this during 2017-03-28 over at https://github.com/bevry/taskgroup/tree/feature/promise

balupton commented 4 years ago

Aiming to tackle this soon.

balupton commented 4 years ago

So ambi support for promises completed a while back https://github.com/bevry/ambi - all that is waiting for this is the implementation of it along with prior work if there is any

As well as support for .then and .catch as alternatives to .done

bevry / taskgroup