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
– jfriend00, Commented Aug 16, 2015 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
– robertklep, Commented Aug 16, 2015 at 19:28
Thanks: I'll have a got at promisifying readFile and then take advantage of Mongoose — Simon H
– Simon H, Commented Aug 16, 2015 at 19:31

Ry- · Accepted Answer · 2015-08-16 19:25:02Z

1

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),
    ]);
});

answered Aug 16, 2015 at 19:25

Ry-♦

226k56 gold badges496 silver badges504 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Simon H Over a year ago

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

Ry- Over a year ago

@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.

Community · Accepted Answer · 2017-05-23 10:33:55Z

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?

jib · Accepted Answer · 2015-08-18 12:06:00Z

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.

Collectives™ on Stack Overflow

Avoiding Promise anti-patterns with node asynchronous functions

3 Answers 3

2 Comments

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

2 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related