Look for Promise bluebird code review for node.js

Question

When and where need to use new Promise(Function<Function resolve, Function reject> resolver) -> Promise

My Sample code:

userInfo.js

var Promise = require('bluebird');
var winston = require('winston');
var _ = require('lodash');
var request = Promise.promisify(require("request"));

exports.getWeather = function (data) {
    var cityName = data.userProfile.city;
    return request("http://0.0.0.0:3003/api/Weather/byCity?city=" + cityName).spread(function (res, body) {
        var result = JSON.parse(body).data;
        return _.merge(data, result);
    });
};

exports.getUserProfile = function (userId) {
    return new Promise(function (resolve, reject) {
        request("http://0.0.0.0:3003/api/UserProfile/getUserProfile?id=" + userId).spread(function (res, body) {
            var result = JSON.parse(body).data;
            resolve(result);
        });
    })

};

exports.getEvents = function (data) {
    var cityName = data.userProfile.city;
    return request("http://0.0.0.0:3003/api/Events/byCity?city=" + cityName).spread(function (res, body) {
        var result = JSON.parse(body).data;
        return _.merge(data, result);
    });
};

exports.getFashion = function (data) {
    var gender = data.userProfile.gender;
    return request("http://0.0.0.0:3003/api/Fashion/byGender?gender=" + gender).spread(function (res, body) {
        var result = JSON.parse(body).data;
        return _.merge(data, result);
    });
};

exports.displayDetail = function (data) {
    console.log(data);
};

Above code I try call in 2 way in promise

getUserProfile.js

var userInfo = require('./userInfo');
    module.exports = function(){
       return userInfo.getUserProfile(3)
                    .then(userInfo.getFashion)
                    .then(userInfo.getEvents)
                    .then(userInfo.getWeather)
                    .then(userInfo.displayDetail)
                    .catch(function (e) {
                        console.log('Error:');
                        console.error(e.stack)
                    })
                    .finally(function () {
                        console.log('done');
                    });

    }

2nd way:

getUserInformation.js

var userInfo = require('./userInfo');
     module.exports = function () {
        return new Promise(function (resolve, reject) {

             resolve(3);
        })
            .then(userInfo.getUserProfile)
                .then(userInfo.getFashion)
                .then(userInfo.getEvents)
                .then(userInfo.getWeather)
                .then(userInfo.displayDetail)
                .catch(function (e) {
                    console.log('Error:');
                    console.error(e.stack)
                })
                .finally(function () {
                    console.log('done');
                });
    };

getDetails.js

var userInfo = require('./getUserInformation');
    userInfo()
    .then(function(){
            console.log('getDetails done')
        })
        .catch(function (e) {
            console.log('Error:');
            console.error(e.stack)
        })
        .finally(function () {
            console.log('done');
        });

please let me know what the difference and is there any issues by using these way?

Both are, assuming the other approach is to use node.js style callbacks. — Kevin B, Aug 07 '15 at 19:33
but creating dummy new Promise and then doing chaining is right? right what happening in background.. and is there any performance issue is there? — Elankeeran, Aug 07 '15 at 19:35
I see no reason for you to do what was mentioned in `2nd way`. Why would that even be a consideration. To be clear, though, it isn't **wrong**, it's just not necessary. — Kevin B, Aug 07 '15 at 19:36
what wrong in 2nd way event i am getting same output. if you explain in detail that will very helpful for me — Elankeeran, Aug 07 '15 at 19:39
Like I said, it isn't **wrong**, it's just not necessary. Having the dummy promise is no different from not having it other than it's extra. — Kevin B, Aug 07 '15 at 19:40
When you're working with something that doesn't implement the promise api. — Kevin B, Aug 07 '15 at 19:42
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/85473/discussion-between-elankeeran-and-kevin-b). — Elankeeran, Aug 07 '15 at 19:43

score 1 · Accepted Answer · edited May 23 '17 at 12:29

exports.getUserProfile = function (userId) {
    return new Promise(function (resolve, reject) {
        request("http://0.0.0.0:3003/api/UserProfile/getUserProfile?id=" + userId).spread(function (res, body) {
            var result = JSON.parse(body).data;
            resolve(result);
        });
    })
};

Please don't do this. Just return from the callback, and return the promise created by then, like you have done it in your other three methods.

return userInfo.getUserProfile(3)
.then(…)

vs.

return new Promise(function (resolve, reject) {
    resolve(3);
})
.then(userInfo.getUserProfile)
.then(…)

Well, the first one is much more readable and concise. They're pretty much equivalent except for the case that getUserProfile does throw synchronously, which it shouldn't anyway. Also in the first case getUserProfile is invoked as a method on userInfo, while in the second case it's just a callback function, the this in the calls will be different.

The second pattern can be tremendously simplified though by using Promise.resolve instead of the new Promise constructor:

return Promise.resolve(3)
.then(userInfo.getUserProfile)
.then(…)

This is totally fine, and aligns better with the rest of the chain. Speaking of which, …

.then(userInfo.getFashion)
.then(userInfo.getEvents)
.then(userInfo.getWeather)
where each of the functions returns a promise that resolves with additional data merged into its argument

is not exactly the best way to solve this. Yes, it ensures that these three functions are called after each other, and is an acceptable pattern for that case. However, in your case you're mixing the request calls to the API with that argument-extraction and result-merging in the same function; which by the separation of concerns you shouldn't. Rather make the functions pure

exports.… = function (arg) {
    return request("http://0.0.0.0:3003/api/…?…=" + arg).spread(function (res, body) {
        return JSON.parse(body).data;
    });
};

And now you can combine them separately - and not only in sequence, but also in parallel:

userInfo.getUserProfile(3)
.then(function(data) {
    var p = data.userProfile;
    return Promise.prop({
         userProfile: 0,
         fashion: userInfo.getFashion(p.gender), // `\
         events: userInfo.getEvents(p.city),     //   }=> execute requests in parallel
         weather: userInfo.getWeather(p.city)    // ./
    });
})
.then(userInfo.displayDetail)
.catch(function (e) {
     console.error('Error:', e.stack)
});

score 0 · Answer 2 · answered Aug 08 '15 at 15:45

The first way is much more readable, and there's no benefit to starting the chain with a promise that returns a constant as in your second way.

They both do effectively the same thing, with one caveat: In your second example (Starting the chain with a Promise), the getUserProfile call will be run on the next tick (Similar to if you'd thrown it in a setTimeout 0) rather than atomically.

Look for Promise bluebird code review for node.js

My Sample code:

2 Answers2