Wait On For Loop In Node Promise

Question

I'm trying to run a sequence of promises using the Q library.

The merge notes function creates a new note in the database and I have to run the functions in order due to some unique constraints.

The promises are running in sequence no problem, but I need to push all of the newNotes into an array in workNotes and then resolve that array.

Everything I have tried resolves the promise before the chain is over.

To clarify the problem, I need to resolve notesList after the chain has completed and every resulting newNote has been pushed to notesList.

workNotes(notes){
    var _this = this;
    var notesList = [];
    return new Promise(
        function(resolve,reject){
            var chain = Q.when();
            notes.forEach(function(note){
                chain = chain.then(function(newNote){
                   _this.notesList.push(newNote);
                   return _this.mergeNotes(note);
                 });
             });
            resolve(notesList)
        }          
    );
}


mergeNotes(notes){
    return new Promise(
        function(resolve,reject){
            doSomething(note)
            .then(function(newNote){
             resolve(newNote);
            })   
         }       
    );
}

If your `doSomething()` is asynchronous, then you need to pass it a callback and use the completion callback to call `resolve()`. Otherwise, you're resolving the promise BEFORE the asynchronous operation is done. If this is not your problem, then please describe in more detail exactly what you want help with. — jfriend00, Jun 01 '17 at 00:22
FYI, a typical design pattern for processing an array in sequence uses `.reduce()`. You can see that in the 3rd and 4th code examples here: [How to synchronize a sequence of promises?](https://stackoverflow.com/questions/29880715/how-to-synchronize-a-sequence-of-promises/29906506#29906506). — jfriend00, Jun 01 '17 at 00:24
Also, not sure why you're using `Q.when()` when `Promise.resolve()` is apparently available (since you're using `new Promise()`). — jfriend00, Jun 01 '17 at 00:25
Your `workNotes` never calls `resolve` or `reject`? And it looks very much like the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! — Bergi, Jun 01 '17 at 00:31
Where's that array that you want to push to? What did you try? — Bergi, Jun 01 '17 at 00:33
@jfriend00 doSomething actually returns a promise also. It's working fine. To clarify the problem, I need to resolve notesList after the chain has completed and every resulting newNote has been pushed to notesList. — Jeremy Wagner, Jun 01 '17 at 00:57
`resolve(chain.then(() => notesList))`? You might want to put this on [CodeReview.SE] afterwards, though. — Ry-, Jun 01 '17 at 01:00

jfriend00 · Answer 1 · 2017-06-01T01:07:32.590

Change mergeNotes() to return the new promise:

mergeNotes(notes){
    return doSomething(note);
}

You were returning a promise, but it was not connected in any way to the doSomething() promise and thus it didn't wait for that.

Avoid the promise anti-pattern of wrapping an existing promise in a newly created promise. Instead, just return the promise you already have.

I'd change the rest of your code to this:

workNotes(notes) {
    let allNotes = [];
    return notes.reduce(function(p, note) {
        return p.then(function() {
            return mergeNotes(note);
        }).then(function(newNote) {
            allNotes.push(newNote);
        });
    }, Promise.resolve()).then(function() {
        return allNotes;
    });
}

With the Bluebird promise library, you could take advantage of Promise.mapSeries() which processes an array in sequence and returns a resolved array which is exactly what you need:

workNotes(notes) {
    return Promise.mapSeries(notes, function(note) {
        return mergeNotes(note);
    });
}

The resolved value of the promise returned by workNotes() will be an array of notes.

@Bergi - I assume the real code does something more than that in there. But, yes if that's all there is, then there's no need for a wrapping function. `doSomething()` can just be called directly. — jfriend00, Jun 01 '17 at 01:04

score 0 · Accepted Answer · answered Jun 01 '17 at 01:02

Drop the useless _this. (notice that this is not about scope!), avoid the Promise constructor antipattern, swap the order of calls to push and mergeNotes, and use reduce instead of forEach to fold an array to a single value:

function workNotes(notes) {
    var notesList = [];
    return notes.reduce(function(chain, note) {
        return chain.then(function() {
            return mergeNotes(note);
        }).then(function(newNote){
            notesList.push(newNote);
        });
    }, Q.when()).then(function() {
        return notesList;
    });
}

Thanks, @Bergi. This is perfect. – Jeremy Wagner Jun 01 '17 at 01:57 — Jeremy Wagner, Jun 01 '17 at 01:57

Wait On For Loop In Node Promise

2 Answers2