NodeJS promise loop through all files in directory

Question

I am trying to re-organise my mp3 collection based on its id3 meta data into folders based on the artist. I'm new to nodeJs and more importantly promises. I was hoping I could get some feedback on my script. I can manage the creating of folders and copying of files quite easily, but I'm a little unsure on the async reading and looping part (shown below).

It does run, but almost crashes my computer when I run it on the full directory.

Thanks in advance for any suggestions or advice.

const fs = require('fs');
const NodeID3 = require('node-id3-promise')
const util = require("util");

const testFolder = '/home/james/Music';
const reorderedFolder = '/home/james/Music/reordered';

const existsProm = util.promisify(fs.access);
const statsProm = util.promisify(fs.stat);
const mkdirProm = util.promisify(fs.mkdir);
const copyFileProm = util.promisify(fs.copyFile);
const removeFileProm = util.promisify(fs.rm);
const readAllFiles = util.promisify(fs.readdir);

const isDelete = false;

async function getFiles(){
    const files = await readAllFiles(testFolder);
    return Promise.all(files);
}

(async ()=>{

    const files = await getFiles();

    files.forEach(file => {
        var sourceFilePath = testFolder + "/" + file;

        Promise.resolve(sourceFilePath)
            .then(filePath =>{
                //Return an array of promises. a) does file exist, b) filePath
                return Promise.all([existsProm(filePath), Promise.resolve(filePath)]);
            })
            .catch(err =>{
                //File wasn't found. Kill it.
                throw new Error("File not found");
            })
            .then(fileExistsAndFilePath => {
                //get tags for existing file path.
                return Promise.all([NodeID3.read(fileExistsAndFilePath[1]), fileExistsAndFilePath[1]])
            })
            .then(tagAndFilePath => {
                //get the id3 tag info and create an object from it.
                var tagDetails = {};
                tagDetails.artist = tagAndFilePath[0].artist;
                tagDetails.title = tagAndFilePath[0].title;
                tagDetails.filePath = tagAndFilePath[1];

                return tagDetails;
            })
            .then(tagDetails => {
                //Create file paths
                if(tagDetails != null && tagDetails.artist != null && tagDetails.title != null){
                    var fileAndFolder = {};
                    fileAndFolder.existingFilePath = tagDetails.filePath;
                    fileAndFolder.newFolderPath = reorderedFolder + "/" + tagDetails.artist;
                    fileAndFolder.newFilePath = fileAndFolder.newFolderPath + "/" + tagDetails.title + ".mp3";
                    return fileAndFolder;
                }else{
                    throw new Error("No artist");
                }
            })
            .then(fileAndFolder => {
                //Create the directory, if it exists, ignore the error
                return Promise.all([mkdirProm(fileAndFolder.newFolderPath).catch(err => {}), fileAndFolder]);
            })
            .then(mdkirAndFileAndFolder => {
                //Copy the file to the new location
                return Promise.all([
                    copyFileProm(mdkirAndFileAndFolder[1].existingFilePath, mdkirAndFileAndFolder[1].newFilePath).catch(err => {
                        console.log(err);
                        throw new Error("Problem copying file");
                    }),
                    mdkirAndFileAndFolder[1]
                ]);
            })
            .then(copyFileResultAndFileAndFolder => {
                //Remove the old file
                if(isDelete){
                    return Promise.all([
                        removeFileProm(copyFileResultAndFileAndFolder[1].existingFilePath).catch(err => {console.log("Problem deleting file " + err);}),
                        copyFileResultAndFileAndFolder[1].existingFilePath
                    ]);
                }else{
                    return Promise.all([null, copyFileResultAndFileAndFolder[1].existingFilePath]);
                }
            })
            .then(rmFileResult => {
                //Output success message
                console.log(rmFileResult[1] + " = successfully moved.");

                return true;
            })
            .catch(error => {
                if(error.message != "File not found" && error.message != "No artist"){
                    console.error(error);
                }
            })
    });
})()

Answer 1

Use precise variable names getFiles resolves not to an array of Files, but to an array of file name strings. Consider renaming appropriately.

Processing issues Very large amounts of parallel file system accesses can make the system seem unresponsive. One solution to this would be to throttle the number of files that can be processed at any one time. The limit will depend on your system, depending on how much resources you consider acceptable to be taken up by the script. To implement this, instead of processing the files immediately with the big outer forEach, construct an array of functions. Each function, when called, processes a file. Then, iterate over up to the limit (say, 15), and call 15 of those functions. Once each file has finished processing, take another function from the array, if it exists, and call it.

For example, if you put the file processing into a separate function named processFile (to make the flow easier to understand), you could do:

const LIMIT = 15;
const fns = filenames.map(filename => () => processFile(filename));
Promise.all(Array.from(
  { length: LIMIT },
  (_, i) => fns.pop?.() // use optional chaining if there may be less than 15 items in the folder
))
  .then(() => {
    // All done
  });

where processFile has, at the end of it, after the .catch:

.then(() => {
  return fns.pop?.();
});

You could also use an index that gets incremented instead of mutating the array with .pop if you wanted, but it almost certainly won't make a difference.

Also, your existing code could do with a number of improvements:

Use fs.promises - unless you're on an obsolete version of Node, the fs module contains native promisified versions of each API. Eg, instead of

const existsProm = util.promisify(fs.access);

just use fs.promises.access, or do something like:

const fs = require('fs').promises;
// ...
// use fs.access

Only use Promise.all on an array of Promises Since readdir resolves to an array of strings, just return that array instead of wrapping it in a Promise.

Only use Promise.resolve when you don't have a Promise to begin with Here, since the data you're missing at the start of processing is the result of calling fs.access, use that as the intial Promise instead of using Promise.resolve(sourceFilePath).

Use await instead of long .then chains - await makes the code much flatter and easier to make sense of at a glance. It also helps avoid having to pass down arrays of Promise.all when you want to transfer multiple values over the asynchronous chain. It will also help you avoid the following problem:

Don't return from a .then when carrying out synchronous processing - .thens allow you to chain together asynchronous operations. If what you're doing isn't asynchronous, don't use .then. For example, this:

.then(tagAndFilePath => {
    //get the id3 tag info and create an object from it.
    var tagDetails = {};
    tagDetails.artist = tagAndFilePath[0].artist;
    tagDetails.title = tagAndFilePath[0].title;
    tagDetails.filePath = tagAndFilePath[1];

    return tagDetails;
})
.then(tagDetails => {
    // do stuff with tagDetails

should be something more like

.then(([id3Result, filePath]) => {  // destructuring is nice
    const { artist, title } = id3Result;
    const tagDetails = {
        artist,
        title,
        filePath: 
    };
    // do stuff with tagDetails

Don't use var - it's better to declare variables with const, both to indicate that a variable won't be reassigned, and to avoid the unintuitive function-scope of var. (const and let both have block scope instead.) (When you must reassign a variable, use let.)

.catch only once? It looks a bit odd to me to .catch and immediately re-throw. Is there a reason you don't want to leave it as-is and just examine the error object at the end of the chain instead? That way you can have just a single .catch that encompasses all possible errors that arose.

Rewrite

const processFile = async (filename) => {
    try {
        const existingFilePath = testFolder + "/" + file;
        await fs.exists(existingFilePath);
        const { artist, title } = await NodeID3.read(exists);
        if (!artist || !title) {
            throw new Error('No artist or no title');
        }
        const newFolderPath = reorderedFolder + "/" + artist;
        await fs.makeDir(newFolderPath);
        const newFilePath = newFolderPath + "/" + title + ".mp3";
        await fs.copyFile(existingFilePath, newFilePath);
        if (isDelete) {
            await fs.rm(existingFilePath);
        }
        console.log(existingFilePath + " = successfully moved.");
    } catch (error) {
        console.log(error.message);
        // implement custom error handling and logging here if needed
    }
    return fns.pop?.();
};

const LIMIT = 15;
const filenames = await fs.readdir(testFolder);
const fns = filenames.map(filename => () => processFile(filename));
Promise.all(Array.from(
    { length: LIMIT },
    (_, i) => fns.pop?.()
))
    .then(() => { /* All done */ });