Why does the promise resolve first?

Question

Based on a code snippet found here on stackoverflow, I want to read all files in a directory and then proceed. I've added a promise, but this somehow doesn't work.

My directory contains 2 files and the console log output is:
promise resolved
inside filenames
inside filenames
inside readFiles
inside readFiles

function readFiles(dirname, onFileContent, onError) {
    return new Promise((resolve, reject) => {
        fs.readdir(dirname, function(err, filenames) {
            filenames.forEach(function(filename) {

                console.log('inside filenames');

                fs.readFile(dirname + filename, 'utf-8', function(err, content) {
                    onFileContent(filename, content);
                });
            });
        });
    });
}

var data = [];
readFiles('datadir/', function(filename, content) {
    console.log('inside readFiles');
    data.push(filename);
}).then(
    console.log('promise resolved');
    //proceed handling the data-array
);

Your code wouldn't even execute with the semicolon but it looks like you should have `then(function(){` and you have just `then`. Assuming there's no semicolon in your real code, that's the explanation. — Denys Séguret, May 09 '17 at 14:28
I dont see the point of your promise - you're never `resolve`ing it, and you're using callbacks to get the file contents. — Jamiec, May 09 '17 at 14:28
@Jamiec the point of the promise would be to wait until all files are read. — Pete Blank, May 09 '17 at 14:32
I get that - but you're never doing that. Did you want help making this properly promise-based? — Jamiec, May 09 '17 at 14:32
yes please. I wouldn't know where to put the resolve(), can't wrap my head around it. — Pete Blank, May 09 '17 at 14:35
Did you want to resolve with all the file contents, or just the filenames (and handle the contents with a callback as your existing code)? — Jamiec, May 09 '17 at 14:39
with all the file contents, the pushing of the filenames was just for testing purposes. — Pete Blank, May 09 '17 at 14:41

Jamiec · Accepted Answer · 2017-05-10T20:05:48.243

The promise does not "resolve first". The call to console.log executes before your first file is read.

You're never calling resolve on your promise, so the then is never being called. However you're passing the result of console.log to the then. the result of console.log is void.

You can test this by correcting the problem:

readFiles('datadir/', function(filename, content) {
    console.log('inside readFiles');
    data.push(filename);
}).then(function(){ // NOTE: addition of function(){..}
    console.log('promise resolved');
    //proceed handling the data-array
});

And you'll notice the message is never written to the console.

So that's whats wrong - how to fix it. It takes some thinking to wrap your head round the totally async/promise based code in node.

I'm assuming that you want to wait for all the files to have the contents read before resolving your promise. This is a little tricky as you have 2 async calls (reading the list of files, and then individually reading their contents). It may well be easier to wrap the reading of the file into its own promise. Something like this:

function readFile(filePath){
    return new Promise((resolve,reject) => {
       fs.readFile(filePath, "utf-8", function(err,content) => {
           if(err) reject(err)
           else resolve({path:filePath, content:content})
        });
    });
}

Do the same for readdir so as to make that also chainable:

function readDirectory(dir){
    return new Promise((resolve,reject) => {
       fs.readdir(dirname, function(err, filenames) {
          if(err) reject(err);
          else{
             resolve(filenames.map(fn => dir + fn));
          }
      });
    });
}

The reason to do this is that you can then chain on a Promise.all to wait for all the file contents too.

function readFileContents(dirname) {
    return readDirectory(dirname)
            .then(files => 
                 Promise.all(files.map(file => readFile(file))
             );
}

Usage:

readFileContents('datadir/').then(files => {
    files.forEach(file => {
        console.log(file.path, file.content.length);
    });
});

Avoid the [`Promise` constructor antipattern](http://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it) in `readFiles`! You should make a `readDir` function that uses the promise constructor, and then combine it properly with `readFile` into `readFiles`. — Bergi, May 09 '17 at 16:55
@Bergi - I thought thats what I did with my `readFile` method. Can you explain what you mean by "uses the promise constructor"? — Jamiec, May 10 '17 at 14:58
The 2 methods I wrote were just simple `Promise` wrappers around an api call which does not have a promise interface. I don't see much harm, but maybe im misunderstanding. — Jamiec, May 10 '17 at 15:04
The problem is that it doesn't only wrap around the api call but also the `Promise.all(…)` thing (and does handle that wrong). You should factor out the promise wrapper around the `fs.readdir` call in a helper function exactly like you did for `readFile`. The `Promise.all(…)` needs to go inside a `.then(…)` callback. — Bergi, May 10 '17 at 16:45

Why does the promise resolve first?

1 Answers1