Refactoring Complicated Nested Node.js Function

Question

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.

CertainPerformance · Accepted Answer · 2018-09-29 00:38:51Z

2

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.

edited Sep 29, 2018 at 0:38

answered Sep 28, 2018 at 23:54

CertainPerformance

373k55 gold badges354 silver badges359 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

Harrison Cramer Over a year ago

Hmmm, I understand the idea, but when I use your code the "all done" block is not firing for me. Files are downloading though. Am I missing something?

CertainPerformance Over a year ago

It should work, note that the Promise chain spawned by download is being implicitly returned in the .map - you copied that, right? (parentheses, not curly bracket)

Harrison Cramer Over a year ago

It doesn't appear to be a problem with your solution, but for some reason the script is consistently failing to download all of the resources... There are 126 pdf links, which are consistently identified by the request to the first link. But the downloader function is failing to download all of them. Here's my git: github.com/KingOfCramers/federal-register-searcher

CertainPerformance Over a year ago

If the download function is normal, it should throw an error if there's a problem, which means the error will be visible in the upper catch, right?

Harrison Cramer Over a year ago

Perhaps it's a problem with the connection? It's only downloading 84 of the 126 files.... Are you getting the same problem, if you download the git? Please let me know if you're able to help out, thx

|

Collectives™ on Stack Overflow

Refactoring Complicated Nested Node.js Function

1 Answer 1

6 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

6 Comments

Your Answer

Sign up or log in

Post as a guest

Related