Avoiding Promise anti-patterns with node asynchronous functions

Question

I've become aware of Promise anti-patterns and fear I may have fallen for one here (see code extract from within a function). As can be seen, I have Promises nested within two node asynchronous functions. The only way I have been able to expose the inner Promise is by using an outer one. I'd welcome guidance on how to write this more elegantly.

function xyz() {
    return new Promise(function(resolve, reject) {
        return Resto.find({recommendation: {$gte: 0}}, function (err, data) {
            if (err) return reject("makeSitemap: Error reading database");

            return fs.readFile(__dirname + '/../../views/sitemap.jade', function(err, file) {
                if (err) return reject("makeSitemap: Error reading sitemap template");

                [snip]

                resolve(Promise.all([
                    NLPromise('../m/sitemap.xml', map1),
                    NLPromise('../m/sitemap2.xml', map2)
                ]));
            });
        });
    });
}

I've also caught the issue in this plnkr

If you "promisify" everything like `fs.readFile()`, then you can simply return the `Promise.all()` result from the `fs.readFileAsync().then()` handler and you will not need the outer promise that you are manually creating. that scheme also tends to handle errors better too. All your async actions will be chained into one promise that you return. That's how you avoid creating new promises. — jfriend00, Aug 16 '15 at 19:24
Also, if you're using Mongoose, be aware that it already has promise support. So `return Resto.find(...).then(...)` could already work. — robertklep, Aug 16 '15 at 19:28
Thanks: I'll have a got at promisifying readFile and then take advantage of Mongoose — Simon H, Aug 16 '15 at 19:31

score 1 · Answer 1 · answered Aug 16 '15 at 19:25

The best solution is to create promise versions of callback-using functions. bluebird, an excellent promise implementation (better than Node’s native one), has this built-in.

Bluebird.promisifyAll(Resto); // If you can promisify the prototype, that’s better
Bluebird.promisifyAll(fs);

function xyz() {
    return Resto.findAsync({recommendation: {$gte: 0}}).then(function (data) {
        return fs.readFileAsync(__dirname + '/../../views/sitemap.jade').then(function (file) {
            …

            return Bluebird.all([
                NLPromise('../m/sitemap.xml', map1),
                NLPromise('../m/sitemap2.xml', map2)
            ]);
        });
    });
}

Also, if the fs.readFile doesn’t depend on the Resto.findAsync, you should probably run them together:

return Bluebird.all([
    Resto.findAsync({recommendation: {$gte: 0}}),
    fs.readFileAsync(__dirname + '/../../views/sitemap.jade'),
]).spread(function (data, file) {
    …

    return Bluebird.all([
        NLPromise('../m/sitemap.xml', map1),
        NLPromise('../m/sitemap2.xml', map2),
    ]);
});

Thanks - I don't know whether it is better practice to use built in libraries or to use third party ones that happen to be more powerful, but I'll follow the first track for the time being — Simon H, Aug 16 '15 at 19:32
@SimonH: I’d usually prefer built-ins, but bluebird is an exception. It’s faster than native promises, has more useful stack traces, has better error handling, and certainly sports many more features. — Ry-, Aug 16 '15 at 19:33

score 0 · Accepted Answer · edited May 23 '17 at 10:33

Based on minitech's suggestion I wrote:

readFileAsync = function(fname) {
    return new Promise(function(resolve, reject) {
        fs.readFile(fname, function (err, data) {
            if (err) return reject(err);
            resolve(data);
        })
    });
};

And then using robertklep's observation that Mongoose can return a promise, I ended up with:

var datasets;   // seemingly unavoidable semi-global variable with simple Promises
return Resto.find({recommendation: {$gte: 0}})
    .then(function(data) {
        datasets = _.partition(data, function(r) {
            return _.includes([0,1], r.recommendation);
        });

        return readFileAsync(__dirname + '/../../views/sitemap.jade');
    })
    .then(function(file) {
        [snip]

        return Promise.all([
            NLPromise('../m/sitemap.xml', map1),
            NLPromise('../m/sitemap2.xml', map2)
        ]);
    });

I may continue to work on the global variable using How do I access previous promise results in a .then() chain?

score 0 · Answer 3 · answered Aug 18 '15 at 12:06

Since this is tagged es6-promise I wanted to leave an es6 answer that doesn't use extensions:

var xyz = () => new Promise((r, e) => Resto.find({recommendation: {$gte: 0}},
                                                 err => err ? e(err) : r()))
  .then(() => new Promise((r, e) =>
      fs.readFile(__dirname + '/../../views/sitemap.jade',
                  err => err? e(err) : r())))
  // [snip]
  .then(() => Promise.all([
    NLPromise('../m/sitemap.xml', map1),
    NLPromise('../m/sitemap2.xml', map2)
  ]));

Anti-patterns to avoid:

Nesting. Flatten chains whenever possible. Use new Promise narrowly.
Hiding errors. Pass out the real cause of failure whenever possible.
Throwing strings. Throw real error objects as they have stack traces.

Avoiding Promise anti-patterns with node asynchronous functions

3 Answers3