What is wrong with this Promise.all solution

Question

I need to loop through an array of items and check if each item's type matches desired type. Once all the checking are done, add the ones that meet requirement to a dropdown select box. In an array where 2 items meet the requirement, this code checks is only adding the first item to the dropdown always, what is wrong with it?

var promises = [];
var html = "";

for (var i = 0; i < items.length; i++) {
  var promise = new Promise(function(resolve, reject){
    $.ajax({
      url: "url" + items[i], 
      dataType: "json",
      success: function (data) {
        console.log(data); 
        console.log(data.type); // "mytype"
        console.log(typeof data.type); // string
        if(data.type == "mytype") {
          html += "<option value='" + data.id + "'>" + items[i] + "</option>";
          resolve();
        }
      }
    });
  promises.push(promise); 
  });
}

console.log(promises) // (2) [undefined, Promise]

Promise.all(promises).then(() => {
  $("#dropdownBox").html(html);
});

EDIT: someone pointed out that I need to use each instead of forloop to make a closure, I tried it but still does not work. I tried doing

$.each(items, function(index){...}

and

items.forEach(function(index){...}

and modified what's inside the loop accordingly but no luck. This post (JavaScript closure inside loops – simple practical example) does not help me.

because I tried `items.forEach(function(item){... ` and changed what's inside the loop accordingly, I am getting `undefined` for the promises array when i log it out — user8571142, Nov 24 '17 at 23:13
Show what you get for each response. Hard to help without a [mcve] — charlietfl, Nov 24 '17 at 23:33
@charlietfl using the forloop approach, I log data returned from each call and can verify that the ajax calls are going through successfully and retrieving the right data, I also know that the types do match. But anything after the 1st call is not added to the dropdown. Also my promises array becomes `(2) [undefined, Promise]` at the end of the loop — user8571142, Nov 24 '17 at 23:37
part of problem is not resolving if type isn't right. Here's a cleaner approach using the `$.ajax` promise https://jsfiddle.net/jun9odh7/1 — charlietfl, Nov 25 '17 at 00:06

charlietfl · Answer 1 · 2017-11-25T00:29:18.613

One of the issues you have is not resolving the promise if the type doesn't meet condition.

Your for() loop also doesn't create a closure so i won't be what you expect it to be when request completes

Since $.ajax returns a promise here's a less anti-pattern approach using map() to create closure and the promise array

// map array of promises
var promises = items.map((item) => {
  // return $.ajax promise
  return $.ajax({
    url: "url" + item,
    dataType: "json"
  }).then((data) => {
    let html = '';
    if (data.type == "mytype") {
      html += "<option value='" + data.id + "'>" + item + "</option>";
    }
    return html;
  });
});

Promise.all(promises).then((res) => {
  // res is array of  html (or empty) strings returned in each then() above
  $("#dropdownBox").html(res.join(''));
}).catch(err => console.log('At least one request failed'));

Demo

Is there a way to do this using the `each` approach you mentioned — user8571142, Nov 25 '17 at 00:42
Sure...create array like you were doing and push each `$.ajax` promise into that array. Switch out `map` for `each` — charlietfl, Nov 25 '17 at 00:43

What is wrong with this Promise.all solution

1 Answers1