Nested promises in NodeJS

Question

I'm writing a service in NodeJS which retrieves a list of Items. For each one, I have to create a counter that indicates the amount of available items. So, if the item is present in the store, the counter simply increments and resolves the promise (because I'm sure there will be at most one in stock).

Otherwise, if it is in the stock, I have to check the exact number of available pieces.

If it is not one of the two previous cases, I'll resolve the promise and I pass to the next item.

The problem is in the second case, because before resolve the main promise (of the current item) I have to wait for the call to retrieve the part counter in the warehouse is over. As it is an asynchronous code, when it currently enter in the second "else", it triggers the call for the piece counter in the stock and immediately resolves the promise, without waiting for the end of the call. How can I solve this concatenation of promises?

This is the code:

let promises: any[] = [];
for (let i = 0, itemsLength = itemList.length; i < itemsLength; i++) {
let currentItem = itemList[i];
promises.push(
    new Promise(function(resolve: Function, reject: Function) {
        act({
            ...call the service to retrieve the item list
        })
        .then(function(senecaResponse: SenecaResponse < any > ) {
            if (senecaResponse && senecaResponse.error) {
                reject(new Error(senecaResponse.error));
            }
            currentItem.itemDetails = senecaResponse.response;

            currentItem.counters = {
                available: 0
            };

            if (currentItem.itemDetails && currentItem.itemDetails.length) {
                for (let k = 0, detailsLength = currentItem.itemDetails.length; k < detailsLength; k++) {
                    let currentItemDetail = currentItem.itemDetails[k];
                    if (currentItemDetail.val === "IN_THE_STORE") {
                        currentItem.counters.available++;

                        resolve();
                    } else if (currentItemDetail.courseType === "IN_STOCK") {
                        act({
                                ...call the service to retrieve the counter in stock
                            })
                            .then(function(stockResponse: SenecaResponse < any > ) {
                                if (stockResponse && stockResponse.error) {
                                    reject(new Error(stockResponse.error));
                                } else {
                                    currentItem.counters.available = stockResponse.response;
                                }
                                resolve();
                            })
                            .catch(function(error: Error) {
                                options.logger.error(error);
                                reject(error);
                            })
                    } else {
                        resolve();
                    }
                }
            } else {
                resolve();
            }
        })
        .catch(function(error: Error) {
            options.logger.error(error);
            reject(error);
        })
    })
);

}
   return Promise.all(promises);

I think you should refactor your code into several functions. So it would be easier to read and maintain. — Orelsanpls, Dec 01 '17 at 08:23
Also avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! — Bergi, Dec 01 '17 at 08:26
there seems to be many places inside loops that can resolve seemingly a single Promise ... — Jaromanda X, Dec 01 '17 at 08:31
Could you provide the structure of the data that you are working with. IT would be much easier reason about, and write some test code — David Gatti, Dec 01 '17 at 08:37

T.J. Crowder · Accepted Answer · 2021-03-11T10:20:56.660

Remember that you can create chains of promises via then and catch, where each handler transforms the resolution value along the way. Since your act() apparently returns a promise, there's no need for new Promise in your code at all. Instead, just use chains.

The fact you need to do sub-queries if products exist isn't a problem; then (and catch) always return promises, so if you return a simple value from your callback, the promise they create is fulfilled with that value, but if you return a promise, the promise they create is resolved to that promise (they wait for the other promise to settle and settle the same way).

Here's a sketch of how you might do it based on the code in the question:

// Result is a promise for an array of populated items
return Promise.all(itemList.map(currentItem => {
    act(/*...query using `currentItem`...*/)
    .then(senecaResponse => {
        if (!senecaResponse || senecaResponse.error) {
            // Does this really happen? It should reject rather than fulfilling with something invalid.
            throw new Error((senecaResponse && senecaResponse.error) || "Invalid response");
        }
        currentItem.itemDetails = senecaResponse.response;
        currentItem.counters = {
            available: 0
        };
        return Promise.all((currentItem.itemDetails || []).map(currentItemDetail => {
            if (currentItemDetail.courseType === "IN_STOCK") {
                return act(/*...query using `currentItemDetail`...*/).then(stockResponse => {
                    currentItem.counters.available = stockResponse.response;
                });
            }
            if (currentItemDetail.val === "IN_THE_STORE") {
                currentItem.counters.available++;
            }
            // (We don't care what value we return, so the default `undefined` is fine; we
            // don't use the array from `Promise.all`
        }))
        .then(() => currentItem); // <== Note that this means we convert the array `Promise.all`
                                  // fulfills with back into just the `currentItem`
    });
}));

@panagulis72: The code above will not cause that error, you must have introduced changes that cause it. `currentItem` is clearly defined at the top and all the rest of the code is nested within that scope. But regardless, it was meant as a **guide**, not "here's the code, you're welcome." Read the comments, take in what it's doing, and tweak as necessary. — T.J. Crowder, Dec 01 '17 at 09:21

Nested promises in NodeJS

1 Answers1