2

So what I am trying to do is push promises to an array of promises inside a forEach, then after the forEach I want to call Promise.all( array of promises ), however each promise is executed immediately so therefore the Promise.all is never reached.

Below is the code:

function updatePersonsPreferences( personId, preferences ) {
    return new Promise( ( resolve, reject ) => {
        const promises = [];
        const methods = preferences.methods;
        let validationError = null;

        methods.forEach( ( method ) => {

            // I do some validation here using a validation helper which is in another file
    const functionSchema = {
        person_id: { type: "integer", required: true },
        type: { type: "integer", required: true },
        frequency: { type: "integer", required: true },
    };

    const data = {
        person_id: personId,
        type,
        frequency,
    };

    const validationResult = validator.isValidFunctionParameters( functionSchema, data );
    if ( !validationResult.valid )
        validationError = validationResult.error;

            promises.push( updateMethod( personId, method.id, method.status ) ); // This is getting executed immediately when it shouldn't be 
        } ); 

        // This does reject, but the promises are still run
        if ( validationError ) {
            return reject( validationError ); 
        }
        // EDIT: Moved console log below the rejection code above, and it is now logging [ Promise { <pending> }, Promise { <pending> } ]
        console.log( promises );

        // The code below never seems to be reached
        return Promise.all( promises ).then( ( results ) => {
            return resolve();
        }, reject );

    } );
}

function updateMethod( personId, methodId, status ) {
    return new Promise( ( resolve, reject ) => {
        // Does some database logic here
        return Method.update( {
            // Database logic here
        } ).then( ( result ) => 
            return resolve( result );
        }, ( error) => { 
           return reject( error );
        } )
    } );
}
Cœur
  • 37,241
  • 25
  • 195
  • 267
Sam Fullen
  • 67
  • 7
  • I'm having trouble understanding your problem. `updateMethod` returns a Promise which will run asynchronously. `Promise.all` takes an array of promises and waits for them all to finish. As far as I can tell, your code does exactly what it's supposed to do. – Mike Cluck Jun 20 '17 at 16:36
  • 1
    A promise is not "executed". A promise is the asynchronous result of executing something. – Bergi Jun 20 '17 at 16:40
  • 3
    Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! – Bergi Jun 20 '17 at 16:41
  • What makes you think the `Promise.all` is never "reached"? JavaScript executes one line at a time in sequence, so by definition it will be reached. Track it in the debugger. Also, there is no need to create a new promise. This is the "explicit promise constructor anti-pattern". Just return `Promise.all`. You also are aware, I hope, that as your code stands, you are discarding the results of `Promise.all`. –  Jun 20 '17 at 16:42
  • Your code does exactly what you expect it to do (altough that is more complicated than necessary). What makes you think the `Promise.all` is never reached? Does something throw an exception before? Do you catch errors on the returned promise? – Bergi Jun 20 '17 at 16:43
  • Do you want to execute the `updateMethod`-calls in sequence *(after the previous promise has finished)* rather than in paralell? Is that what you're asking for? – Thomas Jun 20 '17 at 17:03
  • Hi everyone, thanks for your incredibly quick replies. I have updated my questions' code in order to help elaborate on my issue. Apologies if I have offended anyone with my code or my explanation, I am still fairly new to Javascript and Promises so I'm still learning! The edit I've added shows how I am trying to validate in the forEach and then after the forEach, reject if there was an error during any of the forEaches, however what is happening is that if there is an error, it will successfully reject but the promises are still being executed, which is what I meant when I said (1/2) – Sam Fullen Jun 20 '17 at 17:11
  • (2/2) when I said the Promise.all was not being reached/used. I thought if I added promises to an array and did Promise.all after I had made sure there were no errors, that the promises would only be run if there were no errors. – Sam Fullen Jun 20 '17 at 17:13
  • @Bergi Hi, sorry if I'm not using the correct terminology and things, I'm still quite new to Javascript and promises. My issue is that the promises in the forEach are being executed immediately instead of when Promise.all is called. I have added a console.log of the promises array after the forEach and before the Promise.all and it isn't logging anything – Sam Fullen Jun 21 '17 at 07:44
  • @MikeC Hi, I've edited my question to elaborate on my issue, in the forEach as well as adding to the promises array, I'm doing some validation. I then check after the forEach if there was a validation error and if there was it should reject and not get to Promise.all, which it is doing, however the Promises are still executed. It seems like they are being executed straight away instead of being pushed to the promises array. I added a console log of the promises array after the foreach and it doesn't log anything – Sam Fullen Jun 21 '17 at 07:49
  • 2
    @SamFullen That's the expected behaviour. Promises are not "being run", they cannot be executed, they are just asynchronous result values. Yes, you are immediately executing your operations in the `forEach`. If you don't want that, use two loops - one to check whether there's an error, then when there is none a second to do the updates. – Bergi Jun 21 '17 at 12:36

2 Answers2

3
// The code below never seems to be reached
    *return* Promise.all( promises ).then( ( results ) => {
        return resolve();
    }, reject );

It looks ok at first glance, but try throwing in the above change and let me know if it works.

edit:

return Promise.all(methods.map(method => {
    return updateMethod( personId, method.id, method.status );
})).then(function(results) {
    return resolve(results);
});
Robert Taussig
  • 581
  • 2
  • 11
  • Many apologies, when I added the code to my question I missed off the 'return', it is actually in my code. – Sam Fullen Jun 20 '17 at 17:04
  • My other suggestion might be just be a cosmetic one, but what about instead of using a forEach and pushing a promise into an array, try: --See edit above. (the above will also return the results, whereas your code is leaving them behind) – Robert Taussig Jun 20 '17 at 17:06
0

Apologies once again for my poor understanding of Javascript and Promises, you've gotta learn somehow eh?

According to @bergi (https://meta.stackexchange.com/users/183280/bergi) this is the expected functionality.

In order to validate before the promises are returned I have just used one forEach to validate and another to add to the promises array as @bergi suggested.

Sam Fullen
  • 67
  • 7