CODEWITHSUNDEEP
javascriptnode.jsasynchronouspromiserefactoring
Harrison Cramer
Harrison Cramer

Reputation: 4496

Refactoring Complicated Nested Node.js Function

I have the following snippet of code below. It currently works, but I'm hoping to optimize/refactor it a bit.

Basically, it fetches JSON data, extracts the urls for a number of PDFs from the response, and then downloads those PDFs into a folder.

I'm hoping to refactor this code in order to process the PDFs once they are all downloaded. Currently, I'm not sure how to do that. There are a lot of nested asynchronous functions going on.

How might I refactor this to allow me to tack on another .then call before my error handler, so that I can then process the PDFs that are downloaded?

const axios = require("axios");
const moment = require("moment");
const fs = require("fs");
const download = require("download");
const mkdirp = require("mkdirp"); //  Makes nested files...
const getDirName = require("path").dirname; // Current directory name...

const today = moment().format("YYYY-MM-DD");

function writeFile(path, contents, cb){
  mkdirp(getDirName(path), function(err){
    if (err) return cb(err)
      fs.writeFile(path, contents, cb)
  })
};

axios.get(`http://federalregister.gov/api/v1/public-inspection-documents.json?conditions%5Bavailable_on%5D=${today}`)
  .then((res) => {
    res.data.results.forEach((item) => {
      download(item.pdf_url).then((data) => {
        writeFile(`${__dirname}/${today}/${item.pdf_file_name}`, data, (err) => {
          if(err){
            console.log(err);
          } else {
            console.log("FILE WRITTEN: ", item.pdf_file_name);
          }
        })
      })
    })
  })
  .catch((err) => {
    console.log("COULD NOT DOWNLOAD FILES: \n", err);
  })

Thanks for any help you all can provide.

P.S. –– When I simply tack on the .then call right now, it fires immediately. This means that my forEach loop is non-blocking? I thought that forEach loops were blocking.

Upvotes: 1

Views: 99

Answers (1)

CertainPerformance
CertainPerformance

Reputation: 370689

The current forEach will run synchronously, and will not wait for the asynchronous operations to complete. You should use .map instead of forEach so you can map each item to its Promise from download. Then, you can use Promise.all on the resulting array, which will resolve once all downloads are complete:

axios.get(`http://federalregister.gov/api/v1/public-inspection-documents.json?conditions%5Bavailable_on%5D=${today}`)
  .then(processResults)
  .catch((err) => {
    console.log("COULD NOT DOWNLOAD FILES: \n", err);
  });
function processResults(res) {
  const downloadPromises = res.data.results.map((item) => (
    download(item.pdf_url).then(data => new Promise((resolve, reject) => {
      writeFile(`${__dirname}/${today}/${item.pdf_file_name}`, data, (err) => {
        if(err) reject(err);
        else resolve(console.log("FILE WRITTEN: ", item.pdf_file_name));
      });
    }))
  ));
  return Promise.all(downloadPromises)
    .then(() => {
      console.log('all done');
    });
}

If you wanted to essentially block the function on each iteration, you would want to use an async function in combination with await instead.

Upvotes: 2

Related Questions