Node JS Promise.all and forEach

Question

I have an array like structure that exposes async methods. The async method calls return array structures that in turn expose more async methods. I am creating another JSON object to store values obtained from this structure and so I need to be careful about keeping track of references in callbacks.

I have coded a brute force solution, but I would like to learn a more idiomatic or clean solution.

1. The pattern should be repeatable for n levels of nesting.
2. I need to use promise.all or some similar technique to determine when to resolve the enclosing routine.
3. Not every element will necessarily involve making an async call. So in a nested promise.all I can't simply make assignments to my JSON array elements based on index. Nevertheless, I do need to use something like promise.all in the nested forEach to ensure that all property assignments have been made prior to resolving the enclosing routine.
4. I am using the bluebird promise lib but this is not a requirement

Here is some partial code -

var jsonItems = [];

items.forEach(function(item){

  var jsonItem = {};
  jsonItem.name = item.name;
  item.getThings().then(function(things){
  // or Promise.all(allItemGetThingCalls, function(things){

    things.forEach(function(thing, index){

      jsonItems[index].thingName = thing.name;
      if(thing.type === 'file'){

        thing.getFile().then(function(file){ //or promise.all?

          jsonItems[index].filesize = file.getSize();

Staging

This is the link to the working source that I want to improve. github.com/pebanfield/change-view-service/blob/master/src/… – Staging

I see in the sample you're using bluebird, bluebird actually makes your life even easier with Promise.map (concurrent) and Promise.each (sequential) in this case, also note Promise.defer is deprecated - the code in my answer shows how to avoid it by returning promises. Promises are all about return values. – Lighthouse

Answer 1

It's pretty straightforward with some simple rules:

• Whenever you create a promise in a then, return it - any promise you don't return will not be waited for outside.
• Whenever you create multiple promises, .all them - that way it waits for all the promises and no error from any of them are silenced.
• Whenever you nest thens, you can typically return in the middle - then chains are usually at most 1 level deep.
• Whenever you perform IO, it should be with a promise - either it should be in a promise or it should use a promise to signal its completion.

And some tips:

• Mapping is better done with .map than with for/push - if you're mapping values with a function, map lets you concisely express the notion of applying actions one by one and aggregating the results.
• Concurrency is better than sequential execution if it's free - it's better to execute things concurrently and wait for them Promise.all than to execute things one after the other - each waiting before the next.

Ok, so let's get started:

var items = [1, 2, 3, 4, 5];
var fn = function asyncMultiplyBy2(v){ // sample async action
    return new Promise(resolve => setTimeout(() => resolve(v * 2), 100));
};
// map over forEach since it returns

var actions = items.map(fn); // run the function over all items

// we now have a promises array and we want to wait for it

var results = Promise.all(actions); // pass array of promises

results.then(data => // or just .then(console.log)
    console.log(data) // [2, 4, 6, 8, 10]
);

// we can nest this of course, as I said, `then` chains:

var res2 = Promise.all([1, 2, 3, 4, 5].map(fn)).then(
    data => Promise.all(data.map(fn))
).then(function(data){
    // the next `then` is executed after the promise has returned from the previous
    // `then` fulfilled, in this case it's an aggregate promise because of 
    // the `.all` 
    return Promise.all(data.map(fn));
}).then(function(data){
    // just for good measure
    return Promise.all(data.map(fn));
});

// now to get the results:

res2.then(function(data){
    console.log(data); // [16, 32, 48, 64, 80]
});

Lighthouse

Ah, some rules from your perspective :-) – Tangible

@Tangible someone should really make a list of these rules and a short background on promises. We can host it at bluebirdjs.com probably. – Lighthouse

since I am not supposed to just say thanks - this example looks good and I do like the map suggestion, however, what to do about a collection of objects where only some have async methods? (My point 3 above) I had an idea that I would abstract the parsing logic for each element into a function and then have it resolve either on the async call response or where was no async call have it simply resolve. Does that make sense? – Staging

I also need to have the map function return both the json object i am building and the result of the async call I need to make so not sure how to do that either - finally the whole thing needs to be recursive since I am walking a directory structure - I'm still chewing on this but paid work is getting in the way :( – Staging

I've implemented some of your suggestions. The code does seem cleaner and all of my tests are passing. It's not obvious how to accept your answer. I research this site all the time, but I don't post questions. – Staging

@Staging meta.stackexchange.com/questions/5234/… StackOverflow has a "meta" site with Q&A about StackOverflow (or StackExchange, the even bigger network in this case ) :) – Lighthouse

Here is the new version of my implementation - github.com/pebanfield/change-view-service/blob/master/src/… – Staging

@Staging that resolver and Promise.defer is [an antipattern](https://mcmap.net/q/36569/-what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it). – Lighthouse

After many years of working with event based code I will have to say that I still do not find promises to be very intuitive and the api is not easy to grok. But perhaps this is the cost of programming for a non-blocking IO design? – Staging

@Staging promises are simple, rather than easy, that is - they're not as familiar as other stuff but once you do grok them they're a lot better to use. Hang tight you'll get it :) – Lighthouse

Yes I know - I have removed all of them but that one. Not sure how to go about it, but I'm happy for now and it's 11pm where I am and getting sleepy. :) I'll double back again and fix that thanks. – Staging

Incidentally - I am trying to apply your other solution #25216850 to remove the last anti-pattern you mentioned. :) – Staging

My conclusion is that removing the deferred anti-pattern in this case does not actually improve code readability which is the anti-pattern argument. github.com/petkaantonov/bluebird/wiki/… In fact, I think it makes it more obscure and idiosyncratic just for the sake of being able to say you don't use an anti-pattern. Anyways, just my opinion. Thanks for your help. – Staging

@Staging it's not just about that, your code has nontrivial error handling bugs otherwise. – Lighthouse

Yes, I'm sure it does. Good call. I haven't added a catch block to any of the promise chains for example. Anything else major that you can quickly call out? I've already taken up a lot of your time so it's ok and thanks again. – Staging

Answer 2

Here's a simple example using reduce. It runs serially, maintains insertion order, and does not require Bluebird.

/**
 * 
 * @param items An array of items.
 * @param fn A function that accepts an item from the array and returns a promise.
 * @returns {Promise}
 */
function forEachPromise(items, fn) {
    return items.reduce(function (promise, item) {
        return promise.then(function () {
            return fn(item);
        });
    }, Promise.resolve());
}

And use it like this:

var items = ['a', 'b', 'c'];

function logItem(item) {
    return new Promise((resolve, reject) => {
        process.nextTick(() => {
            console.log(item);
            resolve();
        })
    });
}

forEachPromise(items, logItem).then(() => {
    console.log('done');
});

We have found it useful to send an optional context into loop. The context is optional and shared by all iterations.

function forEachPromise(items, fn, context) {
    return items.reduce(function (promise, item) {
        return promise.then(function () {
            return fn(item, context);
        });
    }, Promise.resolve());
}

Your promise function would look like this:

function logItem(item, context) {
    return new Promise((resolve, reject) => {
        process.nextTick(() => {
            console.log(item);
            context.itemCount++;
            resolve();
        })
    });
}

Fresh

Answer 3

I had through the same situation. I solved using two Promise.All().

I think was really good solution, so I published it on npm: https://www.npmjs.com/package/promise-foreach

I think your code will be something like this

var promiseForeach = require('promise-foreach')
var jsonItems = [];
promiseForeach.each(jsonItems,
    [function (jsonItems){
        return new Promise(function(resolve, reject){
            if(jsonItems.type === 'file'){
                jsonItems.getFile().then(function(file){ //or promise.all?
                    resolve(file.getSize())
                })
            }
        })
    }],
    function (result, current) {
        return {
            type: current.type,
            size: jsonItems.result[0]
        }
    },
    function (err, newList) {
        if (err) {
            console.error(err)
            return;
        }
        console.log('new jsonItems : ', newList)
    })

Sansen

Answer 4

Just to add to the solution presented, in my case I wanted to fetch multiple data from Firebase for a list of products. Here is how I did it:

useEffect(() => {
  const fn = p => firebase.firestore().doc(`products/${p.id}`).get();
  const actions = data.occasion.products.map(fn);
  const results = Promise.all(actions);
  results.then(data => {
    const newProducts = [];
    data.forEach(p => {
      newProducts.push({ id: p.id, ...p.data() });
    });
    setProducts(newProducts);
  });
}, [data]);

Vilma