4

I want to execute an array of Promises in parallel and then wait until all Promises are executed.

This works:

var promises = [];

objects.forEach(function(object) {

    let promise = new Parse.Promise.as()
    .then(
        function() {
            return destroy(object);
        }
    );

    promises.push(promise);
});

return Parse.Promise.when(promises);

But if I use for (object of objects) {...} instead of objects.forEach(function(object) {...}); it doesn't work. For every Promise in the array the destroy(object); is executed on the first object in the array:

var promises = [];

for (object of objects) {

    let promise = new Parse.Promise.as()
    .then(
        function() {
            return destroy(object);
        }
    );

    promises.push(promise);
});

return Parse.Promise.when(promises);

Why is that?

Manuel
  • 14,274
  • 6
  • 57
  • 130

2 Answers2

4

Yes, you forgot to declare the object variable as local to the loop body (see also the canonical explanation):

var promises = [];
for (let object of objects) {
//   ^^^
    promises.push(new Parse.Promise.as().then(function() {
        return destroy(object);
    }));
}
return Parse.Promise.when(promises);

Of course you should not do this either, you should just use map:

var promises = objects.map(function(object) {
    return new Parse.Promise.as().then(function() {
        return destroy(object);
    });
});
return Parse.Promise.when(promises);
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
0

Edit: I was a bit confused originally, and copy & pasted something I wrote for slack. Sorry about that.

Like @Bergi said, if you need an array of Promises from an array of objects then it's usually better to transform that existing array using .map(). As mentioned, this looks like:

const getPromises = (objects) => {
  return objects.map(object => new Parse.Promise.as()
    .then(() => {
      return destroy(object);
    })
  );
}
// No 'temp' array needed, and most important: promise variables aren't 'lost'

// Then you can similarly use this pattern:
return Parse.Promise.when(getPromises(objects));
// Or this:
return Promise.all(getPromises(objects));

My original answer (below) was a bit ambiguous on its own, hopefully my answer above gives it more context. :)

I avoid using for loops or Array.forEach. When I discovered .forEach() I used it for everything, assuming it was the answer to my for loops mess. I've come to learn both are a code-smell 99.99% of the time. Reason: They typically necessitate temp arrays, and make nested loops really awkward.

While .map() largely solves this when your arrays are same size, 1:1 or 20:20, other Array methods are useful for asymmetric transforms, 20:1 (for example: totaling the cost of 20 products into 1 number, or finding the largest transaction in the list):

.map    - use for 1:1 array transforms, as @bergi suggests.
.reduce - useful for transforming 1 array into ANYTHING else. Need a sum or subtotal? Or results grouped by day? Use .reduce().
.filter - return only items which result in a `true` result
.find   - use to avoid full array scans when only 1 item must be returned. 
.some   - exit array scan returning true at first opportunity

let downloadedMsgs  = emails.map(m => downloadBody(m))
let recipientsCount = emails.reduce((count, m) => count + m.to.length, 0)
let onlyRecentMsgs  = emails.filter(m => m.isNew)
let aRecentMsg      = emails.find(m => m.isNew)
let hasNewMessage   = emails.some(m => m.isNew)
// (Notice last 3 identical predicate fn's with different uses: aka pluripotency)

The other issue in the code is the risk of losing your Promises! This is important, if you ever might have dozens or 100's of objects in your array, triggering an HTTP request in the loop would be pretty unreliable (eventually you'd exhaust available sockets). The best way to throttle this involves returning your Promises.

Dan Levy
  • 1,214
  • 11
  • 14
  • Hi Dan, what exactly do you mean by "code-smell 99.99% of the time?" – Manuel Jul 13 '17 at 08:37
  • Great Question... It's not obvious from a simple example, but the approach is simple... see edits. – Dan Levy Jul 13 '17 at 09:47
  • You mean the 'destroy' is executed before 'Parse.Promise.when'? – Manuel Jul 13 '17 at 12:03
  • Yea. Destroy should be designed into a clousure which does the work of the object, maybe return the `destroy()` method so the calling code can just run this when done: `users.map(u => u.destroy())` – Dan Levy Jul 13 '17 at 19:47
  • Why do you think `destroy` is not called by `Parse.Promise.when()`? To my understanding `destroy` is called only when the first Promise in the chain calls it which is `Parse.Promise.when();`. So the order should be `Parse.Promise.when()` -> `Parse.Promise.as()` -> `destroy()`. – Manuel Jul 14 '17 at 23:07
  • I may need to see a bit more code, but `Parse.Promise` is the type returned by Parse's async methods, so you don't need to normally explicitly create a promise chain this way ("Parse.Promise.when" or "Parse.Promise.as"). Essentially your function's logic should be in a `.then` BEFORE the `.then(destroy)`. Then return the promise array without a `.when`. – Dan Levy Jul 14 '17 at 23:19
  • The change will make each object's lifetime match up better with the actions you are taking on it. (Roughly immediate cleanup.) Don't forget to `return` from all your `.then` functions. – Dan Levy Jul 14 '17 at 23:22
  • Dan, I think you are focusing too much on the code sample. It is just an example to describe the issue, not the actual code which contains many more promises in the array from different sources so no single `.map` will do and promises have to be pushed. Also, `Parse.Promise.when()` is the the same as `Promise.all()` and fulfills when all promises resolve or one rejects. I can assure you by experience that no promise in the promises array is called before `Parse.Promise.when()`, I think you are wrong on this. Thanks. – Manuel Jul 15 '17 at 00:04
  • Hey Manuel, I got confused reading this initially, was trying to expand on the .map example @Bergi included. – Dan Levy Jul 16 '17 at 22:57