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!

Uh, how about declaring your variables `local`,`id`, `url` inside the loop body, just like you did with `i`?! — Bergi, Aug 27 '17 at 13:35
Does `request` actually return a promise? Don't you have to [wrap the request in a promise like this](https://stackoverflow.com/questions/41412512/node-js-promise-request-return)? I ask because I know there's a separate package on npm specifically called `request-promise`. — Andy, Aug 27 '17 at 13:36
Assuming `data` is an array: [Don't use `for…in` enumerations!](https://stackoverflow.com/q/500504/1048572) Using `map` instead will also solve your problem most succinctly. — Bergi, Aug 27 '17 at 13:36
@Andy Why do you think this is not a duplicate? Sure there might also be problems with promisification of `get` and `writeFile`, but those don't appear to be what the OP is asking about. — Bergi, Aug 27 '17 at 14:56
His primary issue: "i'm having issues with passing the parameters into the Promise array": you add a promise to a promise array if there is no promise to add. — Andy, Aug 27 '17 at 14:58

score 1 · Accepted Answer · answered Aug 27 '17 at 15:04

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:

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

Notes:

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.
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.
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.
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.

Thanks for the great answer and the additional notes! It started somewhat structured but got worse and worse :) Still so much to learn, thanks a lot! — Paul Etscheit, Aug 27 '17 at 15:57

Passing parameters to in loop created Promises

1 Answers1