Nodejs for loop change to Synchronous

Question

I am new to nodejs. I have a for loop and it tries upload one file at a time from the filearray. For uploading, it calls a method which has promise pattern. So, the for loop continues to execute without waiting for the promise to be returned and so the order in which the files are uploaded are lost. Could anyone help me with this?

  function uploadFiles(model, files){
    var deferred = Q.defer();
    var result = [];

    async.eachSeries(files, function(currFiles, callback) {
        async.eachSeries(currFiles, function(item, innerCallback) {
            var fieldname = item.fieldname;
            var tmp_path = item.path;
            var _file = fs.readFileSync(tmp_path);
            var fileuploaded = parseService.uploadFiles(model, fieldname,item.originalname, { base64 : new Buffer(_file).toString('base64')});
            fileuploaded.then(function(data) {
                result.push(data).then(function (res){
                    console.log('File upload success');
                    innerCallback();
                }, function(err){
                    console.log('File upload fail');
                    innerCallback();
                });
            }, function(err) {
                if (err) {
                    return deferred.reject(err);
                }
                console.log(result);
                deferred.resolve(result);
            });
    }, function() {
            callback();
        });
        return deferred.promise;
    });
};
        parseService.uploadFiles = function(fieldname, filename, file){
           logger.verbose('On uploadFile');
           var deferred = Q.defer();
           var parseFile = new Parse.File(filename, file);
           parseFile.save().then(function() {
             return deferred.resolve();}, 
             function(error) {
               deferred.reject(err);   
              });
             return deferred.promise;}

This is how my methods look. Currently the for loop keeps running and files are getting uploaded asynchronously and so getting uploaded in wrong order.

Answer 1

Assuming your initial files is an array of objects and you don't want to move to ES7 and transpile your code, then you can simplify your code a lot by only using promises. This uses a .reduce() pattern with promises to serialize the upload to one after another.

function uploadFiles(model, files) {
    var results = [];
    return files.reduce(function(p, item) {
        return p.then(function() {
            return fs.readFileAsync(item.path).then(function(fileData) {
                let uploadData = {base64: new Buffer(fileData).toString('base64')};
                return parseService.uploadFiles(item.fieldname, item.originalname, uploadData).then(function(data) {
                    results.push(data);
                });
            });
        });
    }, Promise.resolve()).then(function() {
        return results;
    });
}

parseService.uploadFiles = function (fieldname, filename, file) {
    logger.verbose('On uploadFile');
    var parseFile = new Parse.File(filename, file);
    return parseFile.save();
}

You would then call uploadFiles() like this:

uploadFiles(model, arrayOfFileObjects).then(function(results) {
     // results array here
}).catch(function(err) {
     // some error here
});

---

This assumes you already have a promisified version of the fs module. If you don't, you can get one using Bluebird like this:

const Promise = require('bluebird');
const fs = Promise.promisify(require('fs'));

Or, you can promisify just this one fs.readFile() function like this:

fs.readFileAsync = function(file, options) {
    return new Promise(function(resolve, reject) {
        fs.readFile(file, options, function(err, data) {
            if (err) return reject(err);
            resolve(data);
        });
    });
}

---

If you use Bluebird for promises, then you can use Promise.mapSeries() like this:

const Promise = require('bluebird');
const fs = Promise.promisify(require('fs'));

function uploadFiles(model, files) {
    return Promise.mapSeries(files, function(item) {
        return fs.readFileAsync(item.path).then(function(fileData) {
            let uploadData = {base64: new Buffer(fileData).toString('base64')};
            return parseService.uploadFiles(fieldname, item.originalname, uploadData).then(function(data) {
                return data;
            });
        });
    });
}

Notes:

1. This implementation assumes your original files argument is an array of objects, each object containing info about a file to upload. If it's something different than that, then please describe in your question what exactly it is.

2. You were calling parseService.uploadFiles() with four arguments, yet the function only accepts three arguments. I corrected that.

3. Note, my implementation of uploadFiles() and parseService.uploadFiles() do not manually create any new promises themselves. They just return promises that are already generated by the functions they use. This avoids the promise anti-pattern you were using where you wrapped an existing promise with another manually created promise. There are many common bugs that occur when people make mistakes doing that and it's completely unnecessary.

4. Returning the results like you are, accumulated all the files in memory in an array. This could consume a bunch of memory if the files are large and does not seem to be required for the functionality here.

5. You pass in the model, but you don't have any code that actually uses it.

6. If you use the Bluebird promise library for this, then you could use Promise.mapSeries() instead of the .reduce() to more simply serialize your operations.

Answer 2

I use babel and async/await to simplify this a lot. Once you have your functions returning promises, you can just do something like this:

import readFile from 'fs-readfile-promise';

async function upload(files) {
  const results = [];

  for (let fname of files) {
    let file = await readFile(fname, 'utf8');
    let parsed = await parse(file);
    results.push(parsed);  
  }
  return results;
}

Also, just a side issue, but the naming is confusing since your uploadFiles parses the files instead of uploading them (it seems). Make sure the names make sense.

Answer 3

var async = require('async');

function uploadFiles(currFiles) {
    var deferred = Q.defer();
    var result = []
    async.eachSeries(currFiles, function(item, callback) {
        var fieldname = item.fieldname;
        var tmp_path = item.path;
        var _file = fs.readFileSync(tmp_path);
        var fileuploaded = parseService.uploadFiles(fieldname, item.originalname, {
            base64: new Buffer(_file).toString('base64')
        });
        fileuploaded.then(function(data) {
            result.push(data).then(function(res) {
                logger.verbose('File upload success');
                callback();
            }, function(err) {
                logger.verbose('File upload fail');
                callback();
            });
        });
    }, function(err) {
        if (err) {
            return deferred.reject(err);
        }
        console.log(result);
        deferred.resolve(result);
    });
    return deferred.promise;
}
parseService.uploadFiles = function(fieldname, filename, file) {
    logger.verbose('On uploadFile');
    var deferred = Q.defer();
    var parseFile = new Parse.File(filename, file);
    parseFile.save().then(function() {
            return deferred.resolve();
        },
        function(error) {
            deferred.reject(err);
        });
    return deferred.promise;
}

1. You can not use normal for loop whenever asynchronous behaviour of javascript is invoked.
2. Since the function uploadFiles is having asynchronous calls so it can not return any value except undefined. So you have to use either callback or promise approach