CODEWITHSUNDEEP
javascriptasynchronousforeachpromise
Joe Scotto
Joe Scotto

Reputation: 10877

Promise.then being resolved before previous .then with foreach completes

I'm running a semi-complex promise chain with a foreach loop in the middle. The issue I'm encountering is that the final .then() is being hit before the foreach loop completes resulting in a completely empty dailyTotals array.

fs.readdir(csvPath)
    .then(files => {
        // Define storage array
        var csvFiles = [];

        // Loop through and remove non-csv
        files.forEach(file => {
            if (file !== "README.md" && file !== ".gitignore") {
                csvFiles.push(file);
            }
        });

        return csvFiles;
    })
    .then(files => {
        var dailyTotals = [];

        files.forEach(filename => {
            const loadedFile = fs.createReadStream(csvPath + filename);

            var totalCases = 0;
            var totalDeaths = 0;
            var totalRecovered = 0;

            papa.parse(loadedFile, {
                header: true,
                worker: true,
                step: r => {
                    totalCases += parseIntegerValue(r.data.Confirmed);
                    totalDeaths += parseIntegerValue(r.data.Deaths);
                    totalRecovered += parseIntegerValue(r.data.Recovered);
                },
                complete: () => {
                    var dailyTotal = {
                        date: filename.replace(".csv", ""),
                        parsed: {
                            confirmed: totalCases,
                            deaths: totalDeaths,
                            recovered: totalRecovered
                        }
                    };

                    dailyTotals.push(dailyTotal);
                }
            });
        });

        return dailyTotals;
    })
    .then(dailyTotals => {
        console.log(dailyTotals);
    });

Is there a way to wait for that foreach loop to complete before resolving to the next .then()? The issue is directly on the foreach and the final console.log(dailyTotals);

Upvotes: 2

Views: 85

Answers (3)

Daniel
Daniel

Reputation: 488

No. you will not get your promise to wait for .forEach to before returning. If your .forEach function includes asynchronous code, you should not be using that. Instead you can go with slightly different approaches:

  1. Use a traditionel for-loop, as it is blocking.
  2. Create a range promises .map, similar to what you did with .forEach, which I would recommend like: 
const promises = [array with your values].map(() => {
   // If you are running in a node env, you can also make the function async instead of returning a promise
   return new Promise((resolve, reject) => {
       // Do what you need to
       if (error) {
          reject(error);
       }

       resolve(<insert the value that should be returned here>);

   });
});

Promise.all(promises).then(() => console.log('all promises done'));

See a working example of this with some simple console.log: https://jsfiddle.net/3ghmsaqj/

Upvotes: 2

FZs
FZs

Reputation: 18619

First of all, I have to mention that there are alternative methods to Array#forEach:

  • Array#map which is intended to create a new array by mapping the original one (the same what you do with .forEach), and
  • Array#filter (its name speaks for itself).

The problem is, that you have a promise chain and a callback-based module. To mix them, use the Promise constructor and make use of Promise.all, to compose them into a single promise:

fs.readdir(csvPath)
    .then(files => {

        // Filter out non-csv
        return files.filter(file => (file !== "README.md" && file !== ".gitignore"))
    })
    .then(files => {
        //Returning a Promise from then handler will be awaited
        return Promise.all(files.map(filename => {
            const loadedFile = fs.createReadStream(csvPath + filename);
            return new Promise(resolve => {
                var totalCases = 0;
                var totalDeaths = 0;
                var totalRecovered = 0;

                papa.parse(loadedFile, {
                    header: true,
                    worker: true,
                    step: r => {
                        totalCases += parseIntegerValue(r.data.Confirmed);
                        totalDeaths += parseIntegerValue(r.data.Deaths);
                        totalRecovered += parseIntegerValue(r.data.Recovered);
                    },
                    complete: () => {
                        resolve( {
                          date: filename.replace(".csv", ""),
                          parsed: {
                            confirmed: totalCases,
                            deaths: totalDeaths,
                            recovered: totalRecovered
                          }
                        });
                    }
                });
            });
        }));
    })
    .then(dailyTotals => {
        console.log(dailyTotals);
    });

You can even omit the first .then handler, as .filter is a synchronous operation, and do everything inside a single .then: 

fs.readdir(csvPath)
    .then(files => {
        //Returning a Promise from then handler will be awaited
        return Promise.all(
            files
                //Filter here
                .filter(file => (file !== "README.md" && file !== ".gitignore"))
                //And map without a second then
                .map(filename => {
                    const loadedFile = fs.createReadStream(csvPath + filename);
                    return new Promise(resolve => {
                        var totalCases = 0;
                        var totalDeaths = 0;
                        var totalRecovered = 0;

                        papa.parse(loadedFile, {
                            header: true,
                            worker: true,
                            step: r => {
                                totalCases += parseIntegerValue(r.data.Confirmed);
                                totalDeaths += parseIntegerValue(r.data.Deaths);
                                totalRecovered += parseIntegerValue(r.data.Recovered);
                            },
                            complete: () => {
                                resolve( {
                                  date: filename.replace(".csv", ""),
                                  parsed: {
                                    confirmed: totalCases,
                                    deaths: totalDeaths,
                                    recovered: totalRecovered
                                  }
                                });
                            }
                        });
                    });
                })
        );
    })
    .then(dailyTotals => {
        console.log(dailyTotals);
    });

Upvotes: 2

andy mccullough
andy mccullough

Reputation: 9571

Looks like your second then has async code, papa.parse() so your for each in the second then will be completing synchronously with empty values, and therefore the final then will be executing with those empty values. You need to only return from the second then block when your async code completes

You will have to resolve promises in the complete callbacks, and use the likes of Promise.all to detect when all the complete callbacks are executed

1) change your for each to a map..

const arrayPromises= files.map(... Etc)

2) return a promise from your map callback.

files.map((file) => new Promise((resolve, reject) => {... Everything from your current foraeach}))

3) revolve your promise from within the complete callback

reolve()

4) use return Promise.all(arrayPromises)

Upvotes: 0

Related Questions