CODEWITHSUNDEEP
javascriptnode.jses6-promise
Simon H
Simon H

Reputation: 21047

Avoiding Promise anti-patterns with node asynchronous functions

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

Upvotes: 3

Views: 359

Answers (3)

jib
jib

Reputation: 42530

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.

Upvotes: 0

Simon H
Simon H

Reputation: 21047

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?

Upvotes: 0

Ry-
Ry-

Reputation: 225281

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

Upvotes: 1

Related Questions