Promise usage inside node module

Question

Im new to promise topic and I wonder if I write this following code as it should be .

We are using at our project bluebird

This is the code:

var start = function (destination ){
    return new Promise(function (resolve, reject) {
        fs.readdir(destination, function (err, values) {
            if (err) {
                reject(err);
            } else {
                values.reverse();
                var flag = true;
                values.forEach(function (file) {
                    if (flag) {
                        flag = false;
                        resolve();
                    } else {
                        fs.unlink(destination + '/' + file, function (err) {
                            if (err) {
                                console.log('Error ' + err);
                                reject(err);
                            } else {
                                console.log('sucess ' + dest + '/' + file);
                                resolve();
                            }
                        });
                    }
                });
            }
        });

    });
};

Answer 1

Easy there tiger, you're working way harder than you need to.

Your logic is:

1. Read all the files in destination
2. Reverse them
3. Save the last file and don't unlink it.
4. Unlink all the other files

Here's an alternative answer to mmm's using coroutines and bluebird:

let Promise = require("bluebird");
let fs = require("fs");
Promise.promisifyAll("fs");

const start = Promise.coroutine(function* (destination) { 
    let files = yield fs.readdirAsync(destination); // 1. read files
    files = files.reverse(); 2. reverse them
    files.shift(); // 3. except the first one
    yield Promise.map(files, f => fs.unlinkAsync(`${destination}/${f}`)); 4. unlink
});

Or, possibly more elegantly:

const start = Promise.coroutine(function* (destination) { 
    let files = yield fs.readdirAsync(destination);
    files.pop();
    yield Promise.map(files, f => fs.unlinkAsync(`${destination}/${f}`));
});

Answer 2

In bluebird, using new Promise is an anti-pattern and should be avoided if possible. Instead, you would use bluebird's promisifyAll() method to promisify fs and take that as a starting point.

For example:

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

var start = function (destination ) {
    return fs.readdirAsync(destination)
        .then(values => values.reverse()) // or `.call("reverse")`
        .then(values => {
            // your logic
        }); // .catch(err => { ... }); // by default bluebird logs errors to the console
};

In your further logic you are trying to fulfill or reject the promise multiple times, which can't be done - only the first call to resolve/reject will be taken into consideration here. I think you want to delete all but the latest file. So something like

var start = function (destination ) {
    return fs.readdirAsync(destination)
        .then(values => values.reverse())
        .then(files => {
            files.shift();
            return files;
        })
        .map(file => fs.unlinkAsync(`${destination}/${file}`))
        .then(success => {
            console.log(success) // will hold an array of success messages
            return success;
        });
};

Will do what you want, like it should be done.

---

If you're using node earlier than v4 or are unfamiliar with es6, here's the es5 version:

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

var start = function start(destination) {
    return fs.readdirAsync(destination).then(function (values) {
        return values.reverse();
    }).then(function (files) {
        files.shift();
        return files;
    }).map(function (file) {
        return fs.unlinkAsync(destination + '/' + file);
    }).catch(function (err) {
        console.log(err);
    });
};