0

I'm trying to return an array after a forEach loop has finished adding items to it. I have tried several methods but they all either return an empty array or an unresolved promise.

My Code

class handleFiles {
  constructor() {
    this.promiseFiles = [];
    this.promiseFolders = [];
  }
  async returnFilesAndFolders() {
    fs.readdir("uploads", (err, files) => {
      files.forEach((file) => {
        if (fs.statSync(path.resolve(`uploads/${file}`)).isFile()) {
          this.promiseFiles.push(file);
        } else if (fs.statSync(path.resolve(`uploads/${file}`)).isDirectory()) {
          this.promiseFolders.push(file);
        } else {
        }
      });
    });
    let combined = {
      sendFiles: Promise.all(this.promiseFiles),
      sendFolders: Promise.all(this.promiseFolders),
    };
    return combined;
  }
}

Calling the class

app.get("/:dir?", (req, res) => {
  let dir = req.params.dir;
  console.log(dir);
  let fileHandler = new handleFiles();

  fileHandler.returnFilesAndFolders().then((data) => {
    console.log(data);
    res.render("index", {
      data: data,
    });
  });
});
rishi
  • 652
  • 1
  • 6
  • 20
  • Readdir returns a promise so you can `await fs.readdir...` – Charlie Bamford May 04 '21 at 19:56
  • @Charles Bamford I'm trying to separate it into files and folders. – rishi May 04 '21 at 19:57
  • `returnFilesAndFolders()` returns an array with two promises. `Array` does not have a `then` method. – Sergiu Paraschiv May 04 '21 at 19:58
  • 1
    `let combined = {` happens before your arrays have been populated. it needs to happen after. promisify the readdir and await it or return the promise. – Kevin B May 04 '21 at 19:58
  • @ Kevin B How do I make it happen after? – rishi May 04 '21 at 19:59
  • 1
    promisify the readdir and await it or return it – Kevin B May 04 '21 at 20:00
  • @Kevin B Could you add an answer? I'm afraid I only know this basic amount of async coding. – rishi May 04 '21 at 20:01
  • Does this help? https://stackoverflow.com/questions/14220321/how-do-i-return-the-response-from-an-asynchronous-call/14220323#14220323 – Kevin B May 04 '21 at 20:06
  • @Kevin B The thing is I've gone through several such answers, and tried various suggested approaches, but I can't seem to fix the issue. After you pointed out that the combined is being called before my arrays are populated, I seem to understand why its returning as empty but I dont know how to solve the issue. – rishi May 04 '21 at 20:10

2 Answers2

0

Your original implementation has several timing problems. Your .forEach() loop does not wait for any of the async operations inside it to complete so you end up calling Promise.all() before the arrays are filled. I would suggest using the fs operations that have built-in promise support and also use the withFileTypes option with fs.promises.readdir():

class handleFiles {
  constructor() {
    this.promiseFiles = [];
    this.promiseFolders = [];
  }
  async getFilesAndFolders() {
      let files = await fs.promises.readdir("uploads", {withFileTypes: true});
      for (let file of files) {
          if (file.isFile()) {
              this.promiseFiles.push(file.name);
          } else if (file.isDirectory()) {
              this.promiseFolders.push(file.name);
          }
      }
      return {
          sendFiles: this.promiseFiles,
          sendFolders: this.promiseFolders
      }
  }
}

Note: this implementation is a bit odd in that it is BOTH filling this.promiseFiles and this.promiseFolders and is returning a promise that resolves to an object containing that data. I'd really suggest you do one or the other. Also, because of this side-effect programming of adding to this.promiseFiles and this.promiseFolders, if you call getFilesAndFolders() more than once, it will create duplicates.

Perhaps you don't need this data in your instance data at all and the method can just return a promise that resolves to the object as so:

class handleFiles {
  constructor() {
  }
  async fillFilesAndFolders() {
      const sendFiles = [], sendFolders = [];
      let files = await fs.promises.readdir("uploads", {withFileTypes: true});
      for (let file of files) {
          if (file.isFile()) {
              sendFiles.push(file.name);
          } else if (file.isDirectory()) {
              sendFolders.push(file.name);
          }
      }
      return {
          sendFiles,
          sendFolders
      };
  }
}

But, now this is just a static method that doesn't really have anything to do with the class so perhaps it could just be a stand-alone function or a static method.

Step-by-step Description in the 2nd solution:

  1. Declare local variables promiseFolders and promiseFiles.
  2. Call await fs.promises.readdir() so we can use the promise interface for readdir.
  3. Pass the withFileTypes option so we get access to isFile() and isDirectory() without making an additional call for every file.
  4. Iterate through the list of files and populate the desired promiseFiles or promiseFolders array with the file's name.
  5. Build the returned data structure and return it (which will be the resolved value of the promise returned from the parent async function.
jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • Thank you so much, your second solution was my initial requirement (which I converted into a function). Do u mind explaining the sequence of whats happening – rishi May 04 '21 at 21:24
  • @rishi - I added a step-by-step description. – jfriend00 May 04 '21 at 21:56
  • Thank you so much, however I still dont understand why your `return` runs after the `for loop` while in my code didn't. – rishi May 04 '21 at 22:02
  • @rishi - You have asynchronous operations inside your `.forEach()` loop. That loop does not pause for those asynchronous operations - it just launches them all in parallel, the loop finishes and then the asynchronous operations finish some time later. I'm using `await` with a single asynchronous operation that returns a promise and I have no asynchronous operations inside the loop. – jfriend00 May 05 '21 at 00:35
  • @rishi - You can have asynchronous operations inside a loop, but if you want the loop to wait for them, you must use a plain `for` or `while` loop and you must use `await` with asynchronous operations that return promises. `.forEach()` is not promise-aware so it cannot be used to sequence promises. In fact, I'd argue, `.forEach()` should basically be deprecated now because a `for` loop gives you so much more control and we have block scope and this is all even more true with asynchronous operations. Also a `for` loop can be more optimized by the interpreter too. – jfriend00 May 05 '21 at 00:37
-2
async returnFilesAndFolders() {
  // Return the fs.readdir promise.
  return fs.readdir("uploads").then((files) => {

    // Collect the files into files and folders.
    return files.reduce((acc, file) => {
      // Get the status of the file.
      const stat = fs.statSync(path.resolve(`uploads/${file}`));

      if (stat.isFile()) {
        acc.sendFiles.push(file);
      }
      elseif (stat.isDirectory()) {
        acc.sendFolders.push(file);
      }

      return acc;
    }, {sendFiles: [], sendFolders: []})
  });
}

file is just a string, not a promise, so it doesn't need to be resolved. fs.readdir returns a promise, so it can be used with .then. The intermediary promiseFiles and promiseFolders will be empty when the process exits, but they aren't needed because file is not a promise.

Charlie Bamford
  • 1,268
  • 5
  • 18