Why is Promise returning (resolve) an empty object in Node.js?

Question

I recently ran into problem while executing a node.js file. I will post the code and explain what the problem is.

I have 2 files namely testit.js and test.js

In test.js, I am passing an array object, containing the file paths of textfiles, to the testit.js using module.exports

["a.txt","b.txt"]

In testit.js, module.exports.text accepts an array object of filenames,
processes each of it through Object.keys(texts).forEach,
reads each of the returned buffer values through readFile,
returns the text containing in that buffer through takeAction
and stores it in an array object newtexts.

But when the newtexts is resolved and call returns to then(), in which newtexts is being printed on commandline, it returns an EMPTY ARRAY OBJECT rather than returning an array object of file contents of each those files.

Can someone explain me where I went wrong in the code? Thanks a ton in advance.

test.js

var testit = require('./testit');
var texts = ["a.txt","b.txt"];

testit.text(texts).then(function(newtexts){
  console.log(newtexts);
});

testit.js

var Promise = require('bluebird');
var S = require('string');
var fs = require("fs");

module.exports.text = function(text){

    var texts = text;
    var length = Object.keys(texts).length;

    return new Promise(function(resolve, reject){
        var newtexts = [];

        var takeAction = function(text) {
            return text.toString();
        }

        var readFile = function (filename, enc){
                return new Promise(function (resolve, reject){

                    fs.readFile(filename, enc, function (err, buffer){
                          if(err)
                            reject(err);
                          else
                            resolve(buffer);
                    });
                });
        }

        Object.keys(texts).forEach(function(key){

            readFile(texts[key]).then(function(text){
                 newtexts[key] = takeAction(text);
            });
        });          

        resolve(newtexts);
    });
}

Answer 1

You need to actually wait for all the readFile Promises to resolve before you resolve the overall Promise.

Replace

    Object.keys(texts).forEach(function(key){

        readFile(texts[key]).then(function(text){
             newtexts[key] = takeAction(text);
        });
    });          

    resolve(newtexts);

With something like this:

var textPromises = texts.map( function (fileName) {
    return readFile(fileName).then( function (text) {
        newtexts[fileName] = takeAction(text);
    });
});

Promise.all(textPromises).then( function () {
    resolve(newtexts);
});

The basic idea here is to store the Promise returned by each call of readFile into an array (or more accurately, we store a Promise that resolves after readFile is finished and after the result is processed and stored into newtexts), and only when all of the Promises in the array have resolved do we resolve the promise we return from this function.

Answer 2

Unnecessary promises are worth avoiding as they are expensive operations. You might like to consider the following issues :

• The outer new Promise() wrapper is unnecessary because you can return the promise returned by Promise.all(promises).... That will not only get rid of an unnecessary promise, but also allow errors to propagate to the caller. Note that in your own code, the outer promises's reject is never called.
• Returning a new promise from takeAction() would reduce efficiency twice over; firstly by needing to create a promise, and secondly by needing another .then() (hence yet another promise) to access the result. If operations are synchronous, try to keep them synchronous.
• .then()s in the .map() loop can be avoided completely by shifting synchronous handling of buffer into readFile() (suitably renamed). Again, keep synchronous operations synchronous.

Try this :

module.exports.text = function (fileNames) {
    var newtexts = {}; // <<<<<<< Object not Array.

    function takeAction(key, buffer) { // <<<<<<< takeAction now accepts key and buffer
        newtexts[key] = buffer.toString(); // <<<<<<< make the assignment here
    }

    function readFileAndTakeAction(key) {
        return new Promise(function (resolve, reject) {
            fs.readFile(filenames[key], null, function (err, buffer) {
                if(err) {
                    reject(err);
                } else {
                    takeAction(key, buffer); // <<<<<<< by doing this here, you avoid an extra .then() elsewhere.
                    resolve();
                }
            });
        });
    }
    var promises = Object.keys(fileNames).map(readFileAndTakeAction); 

    // Instead of resolving an outer promise, return Promise.all(...).then(...) 
    return Promise.all(promises).then(function () {
        return newtexts;
    });
}

Changing readFile() from a general-purpose promisifier of fs.readFile() into a specialist arguably robs the code of some elegance, but certainly provides for greater efficiency. Besides, the elegance of .map(readFileAndTakeAction) more than compensates.