How to call Promise function in loop and save its return value

Question

I have created a promise function(using bluebird) called getBasketObject. This function expects a basket as an argument and than return a new basketObject out of it.

basketObject has some variables like tax, total, shipping and productItems. Now, the productItems object has price, name, quantity properties available in it but it doesn't have productImageLink available.

In order to get productImageLink I make a new async call to an endpoint which will get me the product images object. Image Endpoint is also implemented as a Promise.

Now, I loop over the productLineItems and get the value of attributes like name, price, quantity and finally make the call to get image.

Now, if I add

basketObj["products"][productId]["productImageSrc"] = smallImage[0]; my object is never modified and in the final output I don't get image link.

This happens because my getBasketObject returned value before the async call happened. In order to tackle this I added resolve(basketObj); but this returns immediately and am out of the loop.

So, what is the correct way to loop over my product items and get image links for all the products.

exports.getBasketObject = function(basket) {

    return new Promise(function(resolve, reject){

        if (!basket){
            reject("Please give valid basket");
        }
        var basketObj = {};

        if ('order_total' in basket && basket.order_total) {
            basketObj.total = basket.order_total;
        } else if ('product_total' in basket && basket.product_total) {
            basketObj.total = basket.product_total;
        }

        var productLineItems = basket.product_items;
        basketObj["products"] = {};
        for (var key in productLineItems) {
            var productItem = productLineItems[key];
            var productId = productItem.product_id;

            //Async call to get Product Object
            product.getProductObject(productId).then(function(productObj){

                basketObj["products"][productId] = {};
                basketObj["products"][productId]['productQuantity'] = productItem.quantity;
                basketObj["products"][productId]["productName"] = productItem.item_text;
                basketObj["products"][productId]["productPrice"] = productItem.base_price;
                //If promise resolved, get images
                var imageObject = product.getProductImages(productObj[0]);
                var smallImage = imageObject['small'];
                basketObj["products"][productId]["productImageSrc"] = smallImage[0];
                resolve(basketObj); //Acts as a return
            });
        }

    });
};

If I use resolve(basketObject) my final Object looks like

  {
    "total": 95.99,
    "tax": "N/A",
    "shipping": "N/A",
    "products": {
        "701642890706": {
            "productQuantity": 1,
            "productName": "Novelty Stitch Belted Cardigan",
            "productPrice": 95.99,
            "productImageSrc": "image.png"
        }
    }
}

You can see it gets only one product object even if productLineItems has multiple products

Is `productLineItems` an array or an object? The use of a `for in` loop indicates that it is not an array, but because you call it `index` and not `key` looks like it is an array. — t.niese, Apr 09 '16 at 13:33
@t.niese: Its an object, I corrected it to use `key` to avoid confusion. Thanks — RanRag, Apr 09 '16 at 13:36

t.niese · Accepted Answer · 2016-04-09T14:09:08.723

First of all your resolve(basketObj) is not valid because you call resolve multiple times for you Promise, but you must call it only once.

You also should avoid to use string as errors, but always use real error (not only with promises but all the time in javascript).

Instead of your for in loop you can pass Object.keys(productLineItems) in your Promise chain, and then use the .each instead of the for in loop.

That way you can return the Promise introduced by product.getProductObject.

You could rewrite it like that:

exports.getBasketObject = function(basket) {

  var basketObj = {};
  var productLineItems;

  return Promise.resolve(basket)
  .then(function(basket) {
    if( !basket ) {
      throw new Error("Please give valid basket");
    }
    productLineItems = basket.product_items;
  })
  .then(function() {
    if ( 'order_total' in basket && basket.order_total) {
      basketObj.total = basket.order_total;
    } else if ( 'product_total' in basket && basket.product_total) {
      basketObj.total = basket.product_total;
    }

    basketObj.products = {};

    //return the all keys of the productLineItems to be able to iterate over it using promises
    return Object.keys(productLineItems);
  })
  .each(function(key) {
    var productItem = productLineItems[key];
    var productId = productItem.product_id;
    basketObj.products[productId] = {};
    basketObj.products[productId].productQuantity = productItem.quantity;
    basketObj.products[productId].productName = productItem.item_text;
    basketObj.products[productId].productPrice = productItem.base_price;

    //Async call to get Product Object
    return product.getProductObject(productId).then(function(productObj) {
      //If promise resolved, get images
      var imageObject = product.getProductImages(productObj[0]);
      var smallImage = imageObject.small;
      basketObj.products[productId].productImageSrc = smallImage[0];
    });
  })
  .then(function() {
    //  return the basketObj after all  product.getProductObject resolved
    return basketObj;
  });
};

If you don't want to use .each you can write it that way:

exports.getBasketObject = function(basket) {

  var basketObj = {};
  var productLineItems;

  return Promise.resolve(basket)
  .then(function(basket) {
    if( !basket ) {
      throw new Error("Please give valid basket");
    }
    productLineItems = basket.product_items;
  })
  .then(function() {
    if ( 'order_total' in basket && basket.order_total) {
      basketObj.total = basket.order_total;
    } else if ( 'product_total' in basket && basket.product_total) {
      basketObj.total = basket.product_total;
    }

    basketObj.products = {};

    var promises = [];

    Object.keys(productLineItems).forEach(function(key) {
      var productItem = productLineItems[key];
      var productId = productItem.product_id;
      basketObj.products[productId] = {};
      basketObj.products[productId].productQuantity = productItem.quantity;
      basketObj.products[productId].productName = productItem.item_text;
      basketObj.products[productId].productPrice = productItem.base_price;


      promises.push(
        product.getProductObject(productId).then(function(productObj) {
          //If promise resolved, get images
          var imageObject = product.getProductImages(productObj[0]);
          var smallImage = imageObject.small;
          basketObj.products[productId].productImageSrc = smallImage[0];
        });
      );
    });

    return Promise.all(promises);
  })
  .then(function() {
    return basketObj;
  });
};

`promiseObj.each` is not part of standard ES6 `Promise` object. And another disadvantage that `.each` is running the promises in sequence order, not in parallel, like `Promise.all` do — Nick Rassadin, Apr 09 '16 at 13:59
@NickRassadin the OP states that `bluebird` is used, but yes it could be written differently. — t.niese, Apr 09 '16 at 14:02
@RanRag updated that answer based on the comment of Nick, so that it also includes an example without `.each` — t.niese, Apr 09 '16 at 14:09
@RanRag not really but I I would take a look a bit at the anit patterns: [Promise anti patterns](https://github.com/petkaantonov/bluebird/wiki/Promise-anti-patterns), [Promise Patterns & Anti-Patterns](http://www.datchley.name/promise-patterns-anti-patterns) and [Promise Anti-patterns](http://taoofcode.net/promise-anti-patterns/). When ever you have deep nesting (similar to callbacks) you do something wrong and should think about what you can change. — t.niese, Apr 09 '16 at 15:03

score 1 · Answer 2 · answered Apr 09 '16 at 13:49

1

Loop did not finished because you resolve your promise at the first iteration. But you need to wait all the async calls of product.getProductObject to finished. Here Promise.all to help.

...
var asycnCalls = [];
for (var index in productLineItems) {

    ...
    //Async call to get Product Object
    asyncCalls.push(
        product.getProductObject(productId).then(function(productObj){
            //If promise resolved, get images
            var imageObject = product.getProductImages(productObj[0]);
            var smallImage = imageObject['small'];
            basketObj["products"][productId]["productImageSrc"] = smallImage[0];                
        })
    )
} //end of for loop

Promise.all(asyncCalls).then(function(value) { 
    resolve(basketObj); //Acts as a return
}, function(reason) {
    reject(reason);
});

And please make sure product.getProductObject(productId) does really make async calls

answered Apr 09 '16 at 13:49

Nick Rassadin

860
7
7

If you call `resolve` within the callback of a `then` then you most likely used Promises the wrong way. This does not mean that your solution does not work, just that you should not use Promise that way. It is similar to the [`The Forgotten Promise`](http://taoofcode.net/promise-anti-patterns/#the-forgotten-promise:8f173b15e2d19515fdc8ce931ae539c0) – t.niese Apr 09 '16 at 13:54
No, you're wrong. Here method return promise that all concurrent promises will finish. You can't do in any other way. This is far from The Forgotten Promise pattern. – Nick Rassadin Apr 09 '16 at 14:07
The pattern `return new Promise(function(resolve, reject) { /*...*/ Promise.all(asyncCalls).then(function(value) { resolve(basketObj); }, function(reason) { reject(reason); })` is a bad pattern, and thats what you have written in your answer. Either the `return new Promise` is not required, or something before the `Promise.all` is wrong. And thats why it is similar the `The Forgotten Promise`. – t.niese Apr 09 '16 at 14:12
Yes it's definitely right way to change `return new Promise...` to `return Promise.all(...)`, no doubt. But it requires completely rework original code, which does not fit requirement of smooth path in education. – Nick Rassadin Apr 09 '16 at 14:25

How to call Promise function in loop and save its return value

2 Answers2

Linked