Passing parameters to in loop created Promises

Question

Im writing a function that batch downloads images with the request package. The downloading part works, but i'm having issues with passing the parameters into the Promise array.

function downloadImages(data) {
  var promises = [];
  var promise, local, id, url;

  for (var i in data) {
    (function(i) {
      local = "public/images/".concat(data[i].id, ".png");
      url = data[i].img_url
      id = data[i].id

      promise = request.get({
        url: url,
        local: local,
        id: id,
        encoding: 'binary'
      }, function(err, res) {
        if (!err && res.statusCode === 200) {
          fs.writeFile(local, res.body, {
            encoding: 'binary'
          }, (err) => {
            if (!err) {
              doSomething()
            } else {
              console.log("Error Downloading image")
            }
          })
        }
      })
      promises.push(promise)
    })(i)
  }
  Promise.all(promises);
}

When I run this, all parameters in the array resolve to the last entry, so it's downloaded (data.length) times.

I've tried a couple of things but cant get any closer to a solution. Is there something fundamental im doing wrong or something rather simple? Would be very thankful for some help!

Answer 1

I'd suggest simplifying like this:

// make promisified version of fs.writeFile()
fs.writeFileAsync = function(fname, data, options) {
    return new Promise((resolve, reject) => {
        fs.writeFile(fname, data, options, err => {
            if (err) {
                reject(err);
            } else {
                resolve();
            }
        });
    });
}

function downloadImages(data) {
    let promises = data.map(obj => {
        let local = "public/images/".concat(obj.id, ".png");
        let url = obj.img_url;
        let id = obj.id;
        return request.get({url, local, id, encoding: 'binary'}).then(imgData => {
            return fs.writeFileAsync(local, imgData, {encoding: 'binary'}).then(doSomething).catch(err => {
                console.log("Error Downloading image");
                // propagate error after logging it
                throw err;
            });
        });
    });
    return Promise.all(promises);
}

Original problems:

1. Never iterate an array with for/in
2. Variables such as local, url, id were declared at too high a scope so each invocation of the loop would trounce those values
3. Errors were not being properly propagated back to the top level (caller would have had no idea about some errors)

Notes:

1. In the request-promise library, using request.get().then() will automatically check for 2xx status for you and reject if not so you get to remove that code when using .then() instead of plain callback.
2. In general, you don't want to mix promises with plain callbacks because error propagation gets difficult. So, I created a promisified version of fs.writeFile(). If using the Bluebird promise library, it will make promisified version of an entire module at once.
3. Generating a 1-for-1 array from an initial array is what .map() is designed to do. It also gives you a function closure so you don't have to make your own IIFE.
4. Variables used in async callbacks in a loop have to be scoped appropriately (as local as possible) so they don't get overwritten by other iterations of the loop.