Promise.all in Node.js doesn't call the then function

Question

I learn how to use Node and Ecmascript 6.

The aim of my script is to rename a list of png files in a directory. I want to rename only the png files (let'say there is jpg too) and display at the end the number of files renamed. Due to non-blocking nature of Node, it's not so obvious and i decided to take advantage of the opportunity to discover ES6 promise.

'use strict';

const fs = require('fs-extra');

const dir = '/Users/toto/Desktop/png/';
const suffix = '_IMAGE.';
const regex = /(.*)\.(png)$/;
var nbFiles = 0;

// Rename a png file with a suffix
var renameFile = (filename) => {
  return new Promise((resolve, reject) => {
      if(regex.test(filename)){
        let newFileName = filename.replace(regex, '$1' + suffix + '$2');
        fs.rename(dir + filename, dir + newFileName, (err) => {
          let msg = filename + ' => ' + newFileName;

          if (err) {
            console.log('KO : rename of ' + msg);
            reject(err);
          }

          console.log('OK : rename of ' + msg); 
          resolve(nbFiles++);
        });
      }
    });
};

// Read files in a directory and call renameFile + display number of files renamed
fs.readdir(dir, (err, files) => {
    if(err) return console.error(err);

    var promise = Promise.all(
        files.map(renameFile)
    ).then(function(nb){
      console.log('Number of files renamed : ', nb);
    }).catch(function(err){
      console.log('Error ', err);
    });
});

The expected result is to get files renamed and to see the message Number of files renamed.

I get files renamed but i can't see any message (of the then or the catch call). Something is wrong somewhere but debug sessions can't help me.

Thanks for any help!

PS : my environment is Node 5.10 and OS X 10.11.

You really should promisfy at the lowest possible level - only create a promise for `fs.rename` and nothing else - no logic, no logs. Your problem is that your promise never is resolved when the `regex` doesn't match. — Bergi, Apr 01 '16 at 11:53

score 2 · Accepted Answer · edited May 23 '17 at 12:31

2

The problem is that you are creating some promises that stay forever pending (are never resolved): when the regex doesn't match the filename. The Promise.all will wait for them - indefinitely.

You should always promisify at the lowest possible level - in your case fs.rename and fs.readdir - and put no other code inside that function that deals with the "old" callback API. No application logic, no string concatenation, no logging, no nothing.

function rename(from, to) {
    return new Promise((resolve, reject) => {
        fs.rename(from, to, (err, res) => {
            if (err) reject(err);
            else resolve(res);
        });
    });
}
function readdir(from, to) {
    return new Promise((resolve, reject) => {
        fs.readdir(from, to, (err, res) => {
            if (err) reject(err);
            else resolve(res);
        });
    });
}

(If that seems repetitive - it is - write a helper function, or use one from a promise library)

With those, you can now correctly (and easier) implement your script:

const dir = '/Users/toto/Desktop/png/';
const suffix = '_IMAGE.';
const regex = /(.*)\.(png)$/;
readdir(dir).then(files =>
    Promise.all(files.map(filename => ({
        from: dir + filename,
        to:   dir  + filename.replace(regex, '$1' + suffix + '$2')
    })).filter(r => r.from != r.to).map(r => {
        let msg = r.from + " => " + r.to;
        return rename(r.from, r.to).then(() => {
            console.log("OK: " + msg);
        }, err => {
            console.log("KO: " + msg);
            throw err;
        });
    }));
).then(function(res) {
    console.log('Number of files renamed : ', res.length);
}).catch(function(err) {
    console.error('Error ', err);
});

edited May 23 '17 at 12:31

Community

1
1

answered Apr 01 '16 at 12:12

Bergi

630,263
148
957
1,375

interesting code! I like also the fact that you avoid the imperative pattern of 1st testing the regex and 2nd maybe replacing sth. Instead you are filtering out the files that would be "renamed to themselves". – Anton Harald Apr 01 '16 at 12:21
This is some really good code and so much easier to grok what's happening. – Wainage Apr 01 '16 at 12:32
Thankgs Bergi for your quick and elegant answer! I understand that it's better to promisify at the lowest level. My function renameFile did to much things (regex test + rename). I love how you introduce a anonymous object (with from and to properties) to use it afterwards in a functionnal way. I don't think functionnal enough yet. I think that a .catch() is missing in your code after return renam. Can you edit @Bergi for the people in the future? – pom421 Apr 01 '16 at 12:41
@pom421: Yeah, I went for `filter` so that the number of renamed files is the number of results and does not need to be counted manually; but you can try to rewrite your code in the original style with the promisified function and it should work as well. No, there's no `catch` missing, it's the second argument to `then`. – Bergi Apr 01 '16 at 13:05
Oh, ok I didn't know this syntax. Is there a good reason to use it given that you use then/catch later? – pom421 Apr 01 '16 at 13:47
@pom421: Yes, it appropriate here - see [difference between `.then(…).catch(…)` and `.then(…, …)`](http://stackoverflow.com/q/24662289/1048572). We want either callback to execute, not possibly both. – Bergi Apr 01 '16 at 14:35

Promise.all in Node.js doesn't call the then function

1 Answers1