I'm new to node and I'm having an issue with resolving an async Promise. My promise isn't resolving and I'm not sure what I did wrong. I'm still having troubles understanding promises and callbacks so any feedback is helpful.

  var filterFiles = function(){
    return new Promise(function(resolve, reject){
      fs.readdir(rootDir, function(err, files){
        if(err) return console.log(err);

        var task = function(file){
          return new Promise(function(resolve, reject){
            if(! /^\..*/.test(file)){
              fs.stat(rootDir + '/' + file, function(err, stats){
                if(stats.isDirectory()){
                  dirArray.push(file);
                  console.log(dirArray.length);
                  resolve(file);
                }
                if(stats.isFile()){
                  fileArray.push(file);
                  console.log(fileArray.length);
                  resolve(file);
                }
              })
            }

          })
        };

        var actions = files.map(task);

        return Promise.all(actions).then(function(resolve, reject){
          resolve({dirArray: dirArray, fileArray: fileArray});
        });
      })
    })
  }

  filterFiles().then(function(data){
    console.log(data);
    var obj = {
      fileArray: fileArray,
      dirArray: dirArray
    };
    res.send(obj);
  })

I'm new to node and I'm having an issue with resolving an async Promise. My promise isn't resolving and I'm not sure what I did wrong. I'm still having troubles understanding promises and callbacks so any feedback is helpful.

  var filterFiles = function(){
    return new Promise(function(resolve, reject){
      fs.readdir(rootDir, function(err, files){
        if(err) return console.log(err);

        var task = function(file){
          return new Promise(function(resolve, reject){
            if(! /^\..*/.test(file)){
              fs.stat(rootDir + '/' + file, function(err, stats){
                if(stats.isDirectory()){
                  dirArray.push(file);
                  console.log(dirArray.length);
                  resolve(file);
                }
                if(stats.isFile()){
                  fileArray.push(file);
                  console.log(fileArray.length);
                  resolve(file);
                }
              })
            }

          })
        };

        var actions = files.map(task);

        return Promise.all(actions).then(function(resolve, reject){
          resolve({dirArray: dirArray, fileArray: fileArray});
        });
      })
    })
  }

  filterFiles().then(function(data){
    console.log(data);
    var obj = {
      fileArray: fileArray,
      dirArray: dirArray
    };
    res.send(obj);
  })

• javascript
• node.js
• promise

• Avoid the Promise constructor antipattern and never nest new Promise calls! – Bergi Commented Jun 19, 2017 at 22:13
• 1 Your Promise.all(actions).then(function(resolve, reject)) doesn't work because .then only passes a parameter that is the result of the previous promise. As well on the 4th line you use console.log instead of calling reject with the error. You also don't handle an error from fs.stat which can happen when a file doesn't exist. – PaulBGD Commented Jun 19, 2017 at 22:13
• In task there are many else cases where the promise is never resolved – Bergi Commented Jun 19, 2017 at 22:14
• Code here in stackoverflow with lots of indentation is simply not legible with only two spaces per indent. One can't tell what lines up with what. That might work in a code editor that visually shows you indent levels. Doesn't work here. For example, can't tell what level var actions = files.map(task) is at. – jfriend00 Commented Jun 19, 2017 at 22:21

2 Answers 2

It see at least three errors:

1. When you hit this if statement if(! /^\..*/.test(file)){ and it does not execute the if block, then the parent promise is never settled.

2. There is no error handling on fs.stat() so if you get an error on that call, you are ignoring that and will be attempting to use a bad value.

3. The error handling on your call to fs.readdir() is inplete and will leave you with a promise that is never settled (when it should be rejected).

For a robust solution, you really don't want to be mixing promises and callbacks in the same code. It leads to the opportunity for lots of mistakes, particularly with error handling (as you can see you had at least three errors - two of which were in error handling).

If you're going to use Promises, then promisify the async operations you are using at the lowest level and use only promises to control your async code flow. The simplest way I know of to promisify the relevant fs operations is to use the Bluebird promise library with its Promise.promisifyAll(). You don't have to use that library. You could instead manually write promise wrappers for the async operations you're using.

Here's a version of your code using the Bluebird promise library:

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

function filterFiles() {
    return fs.readdirAsync(rootDir).then(function(files) {
        let fileArray = [];
        let dirArray = [];

        // filter out entries that start with .
        files = files.filter(function(f) {
            return !f.startsWith(".");
        });
        return Promise.map(files, function(f) {
            return fs.statAsync(f).then(function(stats) {
                if (stats.isDirectory()) {
                    dirArray.push(f);
                } else {
                    fileArray.push(f);
                }
            });
        }).then(function() {
            // make the resolved value be an object with two properties containing the arrays
            return {dirArray, fileArray};
        });
    });
}



filterFiles().then(function(data) {
    res.json(data);
}).catch(function(err) {
    // put whatever is appropriate here
    res.status(500).end();
});

This was rewritten/restructured with these changes:

1. Use promises for all async operations
2. Fix all error handling to reject the returned promise
3. Filter out files starting with a . synchronously before processing any files (simplifies async processing).
4. Use Promise.map() to process an array of values in parallel.
5. In the filterFiles().then() handler, handle errors
6. You can't res.send() a Javascript object so I used res.json(data) instead (though I'm not sure what exactly you really want to send).
7. Replace regex parison with more efficient and simpler to understand .startsWith().

If you don't want to use the Bluebird promise library, you can make your own promise wrappers for the fs methods you use like this:

fs.readdirAsync = function(dir) {
    return new Promise(function(resolve, reject) {
        fs.readdir(dir, function(err, data) {
            if (err) {
                reject(err);
            } else {
                resolve(data);
            }
        });
    });
}

fs.statAsync = function(f) {
    return new Promise(function(resolve, reject) {
        fs.stat(f, function(err, data) {
            if (err) {
                reject(err);
            } else {
                resolve(data);
            }
        });
    });
}

function filterFiles() {
    return fs.readdirAsync(rootDir).then(function(files) {
        let fileArray = [];
        let dirArray = [];

        // filter out entries that start with .
        files = files.filter(function(f) {
            return !f.startsWith(".");
        });
        return Promise.all(files.map(function(f) {
            return fs.statAsync(f).then(function(stats) {
                if (stats.isDirectory()) {
                    dirArray.push(f);
                } else {
                    fileArray.push(f);
                }
            });
        })).then(function() {
            // make the resolved value be an object with two properties containing the arrays
            return {dirArray, fileArray};
        });
    });
}


filterFiles().then(function(data) {
    res.json(data);
}).catch(function(err) {
    res.status(500).end();
});

The main issue you are having is that outer-most Promise is not resolved or rejected. You can fix this by resolving your Promise.all instead of returning it.

    resolve(
      Promise.all(actions)
        .then(function(resolvedTasks){
          // ... next potential issue is here
          return {dirArray: dirArray, fileArray: fileArray}
        })
    );

(I know, kind of awkward-looking right?)

Next, your return value after the Promise.all resolves is a little weird. In the task function, you're pushing items onto dirArray and fileArray, but they are not declared or assigned in your snippet. I will assume that they are in-scope for this code. In this case, you just need to return your desired object.

Additionally, to make your async code more readable, here are some tips:

• try not to mix callbacks with Promises
• use a Promise library to promisify any code limited to callbacks. Example: bluebird's promisifyAll
• avoid nesting callbacks/promises when possible