7

I want to process a number of promises in Sequence. I have a working piece of code below but I'm wondering if I have over complicated the chaining of promises. I seem to be creating a great deal of new closures and I'm scratching my head wondering if I'm missing something.

Is there a better way to write this function:

'use strict';
addElement("first")
.then(x => {return addElement("second")})
.then(x => { return addElement("third")})
.then(x => { return addElement("fourth")})   

function addElement(elementText){
    var myPromise = new Promise(function(resolve,reject){
        setTimeout(function(){
            var element=document.createElement('H1');
            element.innerText = `${elementText} ${Date.now()}`;
            document.body.appendChild(element);
            resolve();
        }, Math.random() * 2000);
    });
return myPromise;
}
Martin Beeby
  • 4,523
  • 1
  • 26
  • 20
  • 3
    your arrow functions could be simplified - `.then(x => addElement("second"))` - similarly you could be using arrow functions in `addElement` - but I'm not sure why you think you're creating "a great deal of new closures" – Jaromanda X Mar 03 '16 at 16:39
  • I've ran into this issue as well and ended up using `bind` instead, although it feels just as messy (but avoids the extra function wrappers): `.then(addElement.bind(null, "second"))`, etc. – Jesse Kernaghan Mar 03 '16 at 16:40
  • Just wondering if there are redundant promise objects created here. Something like you create 6 promise objects when 3 would suffice? Then already creates a Promise Object which you can't re-use? Let me think up I could be wrong. – Nishant Mar 03 '16 at 17:07
  • @jaromanda ahh you are right... I actually refactored it from originally being: function(){ return addElement("second")} seems that => is short hand for function(){return } rather than just function(){} as I thought. – Martin Beeby Mar 03 '16 at 17:09
  • @Nishant that exactly what was in my head. It feels like there should be an easier way. – Martin Beeby Mar 03 '16 at 17:10
  • I added an answer thought its almost a question. I am just wondering it really isn't possible to reduce the number of promises. It seems like we are creating double promises to be and it isn't really needed. – Nishant Mar 03 '16 at 20:07

6 Answers6

6

@TheToolBox has a nice answer for you.

Just for fun, I'm going to show you an alternative technique that uses generators that gets its inspiration from coroutines.

Promise.prototype.bind = Promise.prototype.then;

const coro = g => {
  const next = x => {
    let {done, value} = g.next(x);
    return done ? value : value.bind(next);
  }
  return next();
}

Using that, your code will look like this

const addElement = elementText =>
  new Promise(resolve => {
    setTimeout(() => {
      var element = document.createElement('H1');
      element.innerText = `${elementText} ${Date.now()}`;
      document.body.appendChild(element);
      resolve();
    }, Math.random() * 2000);
  });

coro(function* () {
  yield addElement('first');
  yield addElement('second');
  yield addElement('third');
  yield addElement('fourth');
}());

There's some pretty interesting things you can do using generators with promises. They're not immediately evident here because your addElement promise doesn't resolve any actual values.


If you actually resolve some values, you could do something like

// sync
const appendChild = (x,y) => x.appendChild(y);

// sync
const createH1 = text => {
  var elem = document.createElement('h1');
  elem.innerText = `${text} ${Date.now()}`;
  return elem;
};

// async
const delay = f =>
  new Promise(resolve => {
    setTimeout(() => resolve(f()), Math.random() * 2000);
  });

// create generator; this time it has a name and accepts an argument
// mix and match sync/async as needed
function* renderHeadings(target) {
  appendChild(target, yield delay(() => createH1('first')));
  appendChild(target, yield delay(() => createH1('second')));
  appendChild(target, yield delay(() => createH1('third')));
  appendChild(target, yield delay(() => createH1('fourth')));
}

// run the generator; set target to document.body
coro(renderHeadings(document.body));

Worth noting, createH1 and appendChild are synchronous functions. This approach effectively allows you to chain normal functions together and blur the lines between what is sync and what is async. It also executes/behaves exactly like the code you originally posted.

So yeah, this last code example might be slightly more interesting.


Lastly,

One distinct advantage the coroutine has over the .then chaining, is that all of the resolved promises can be accessed inside the same scope.

Compare .then chains ...

op1()
  .then(x => op2(x))
  .then(y => op3(y))    // cannot read x here
  .then(z => lastOp(z)) // cannot read x or y here

to the coroutine ...

function* () {
  let x = yield op1(); // can read x
  let y = yield op2(); // can read x and y here
  let z = yield op3(); // can read x, y, and z here
  lastOp([x,y,z]);     // use all 3 values !
}

Of course there are workarounds for this using promises, but oh boy does it get ugly fast...


If you are interested in using generators in this way, I highly suggest you checkout the co project.

And here's an article, Callbacks vs Coroutines, from the creator of co, @tj.

Anyway, I hope you had fun learning about some other techniques ^__^

Community
  • 1
  • 1
Mulan
  • 129,518
  • 31
  • 228
  • 259
4

I am not sure why others left out a simple way out, you could simply use an array and reduce method

let promise, inputArray = ['first', 'second', 'third', 'fourth'];

promise = inputArray.reduce((p, element) => p.then(() => addElement(element)), Promise.resolve());
mido
  • 24,198
  • 15
  • 92
  • 117
3

Your code looks close to the best you can get here. Promises can be a strange structure to get used to, especially as writing promis-ified code can often end up embedding a function in another function. As you can see here, this is a pretty common phrasing to use. There are only two stylistic changes that could possibly be made. Firstly, myPromise is unnecessary and only serves to add a confusing extra line of code. It's simpler just to return the promise directly. Secondly, you can use function binding to simplify your calls at the beginning. It may not be inside the function itself, but it does eliminate several closures. Both changes are shown below:

'use strict';
addElement("first")
.then(addElement.bind(null,"second"))
.then(addElement.bind(null,"third"))
.then(addElement.bind(null,"fourth"))   

function addElement(elementText){
    return new Promise(function(resolve,reject){
        setTimeout(function(){
            var element=document.createElement('H1');
            element.innerText = `${elementText} ${Date.now()}`;
            document.body.appendChild(element);
            resolve();
        }, Math.random() * 2000);
    });
}

It's worth pointing out that, if you were willing to restructure a bit, a slightly more attractive design would take form:

'use strict';
var myWait = waitRand.bind(null,2000);
myWait
  .then(addElement.bind(null, "first"))
  .then(myWait)
  .then(addElement.bind(null, "second"))
  .then(myWait)
  .then(addElement.bind(null, "third"))

function waitRand(millis) {
  return new Promise((resolve, reject) => {
    setTimeout(resolve, Math.random() * millis);
  }
}

function addElement(elementText) {
  var element = document.createElement('h1');
  element.innerText = `${elementText} ${Date.now()}`;
  document.body.appendChild(element);
}

This trades length of promise chain for clarity, as well as having slightly fewer nested levels.

TheToolBox
  • 272
  • 2
  • 10
3

You could simplify the use of your function by making addElement() return a function instead so it can be directly inserted into .then() handlers without having to create the anonymous function:

'use strict';
addElement("first")()
  .then(addElement("second"))
  .then(addElement("third"))
  .then(addElement("fourth"))   

function addElement(elementText){
    return function() {
        return new Promise(function(resolve){
            setTimeout(function(){
                var element=document.createElement('H1');
                element.innerText = `${elementText} ${Date.now()}`;
                document.body.appendChild(element);
                resolve();
            }, Math.random() * 2000);
        });
    }
}
jfriend00
  • 683,504
  • 96
  • 985
  • 979
1

There's not much to be done with regard to the number of closures. Nesting of functions is just something you get used to with js, and the code in the question really isn't that bad.

As others have said, writing addElement() to return a function makes for a neater main promise chain.

Going slightly further, you might consider writing the returned function with an inner promise chain, allowing the (slight) separation of promise resolution from DOM element insertion. This creates no more and no less closures, but is syntactically neater, in particular allowing you to write setTimeout(resolve, Math.random() * 2000);.

'use strict';
addElement("first")
.then(addElement("second"))
.then(addElement("third"))
.then(addElement("fourth"));

function addElement(elementText) {
    return function() {
        return new Promise(function(resolve, reject) {
            setTimeout(resolve, Math.random() * 2000);
        }).then(function() {
            var element = document.createElement('H1');
            document.body.appendChild(element);
            element.innerText = `${elementText} ${Date.now()}`;
        });
    };
}

Maybe it's just me but I find this much more pleasing on the eye, albeit at the cost of an additional .then(), hence an additional promise, per addElement().

Note: If you needed to resolve the promise with a value, you are still afforded the opportunity to do so by returning a value from the chained then's callback.

Going even further, if you want the inserted elements to appear in the demanded order, not the order determined by promise settlement, then you can create/insert elements synchronously, and populate them asynchronously :

function addElement(elementText) {
    var element = document.createElement('H1');
    document.body.appendChild(element);
    return function() {
        return new Promise(function(resolve, reject) {
            setTimeout(resolve, Math.random() * 2000);
        }).then(function() {
            element.innerText = `${elementText} ${Date.now()}`;
        });
    };
}

All that was necessary was to move two lines within addElement(), to change the timing of the insertions whilst leaving the element.innerText = ... line where it was. This is possible whether or not you opt for the inner promise chain.

Roamer-1888
  • 19,138
  • 5
  • 33
  • 44
  • Your first call to `addElement()` needs another `()` after it to actually call the inner function (as shown in my answer). And, you don't need this extra promise to have the items inserted in demanded order. That is already done by other solutions. The inner functions are called in demanded order already. – jfriend00 Mar 04 '16 at 18:42
0

I wrote two methods here :

Sequence = {
    all( steps ) {
        var promise = Promise.resolve(),
            results = [];

        const then = i => {
            promise = promise.then( () => {
                return steps[ i ]().then( value => {
                    results[ i ] = value;
                } );
            } );
        };

        steps.forEach( ( step, i ) => {
            then( i );
        } );

        return promise.then( () => Promise.resolve( results ) );
    },
    race( steps ) {
        return new Promise( ( resolve, reject ) => {
            var promise = Promise.reject();

            const c = i => {
                promise = promise.then( value => {
                    resolve( value );
                } ).catch( () => {
                    return steps[ i ]();
                } );
            };

            steps.forEach( ( step, i ) => {
                c( i );
            } );

            promise.catch( () => {
                reject();
            } );
        } );
    }
};

Sequence.all will run functions in a sequence until all promises in arguments are resolved. And return another Promise object with arguments as an array filled with all resolved values in sequence.

Sequence.all( [ () => {
    return Promise.resolve( 'a' );
}, () => {
    return Promise.resolve( 'b' );
} ] ).then( values => {
    console.log( values ); // output [ 'a', 'b' ]
} );

Sequence.race will run functions in a sequence and stop running while one promise object been resolved.

Sequence.race( [ () => {
    return Promise.reject( 'a' );
}, () => {
    return Promise.resolve( 'b' );
}, () => {
    return Promise.resolve( 'c' );
} ] ).then( values => {
    console.log( values ); // output [ 'a' ]
} );
LCB
  • 971
  • 9
  • 22