Best way to handle the nodejs promise

Question

I am new to nodejs and promises. This is code I wrote by reading some articles but I feel I am not on correct path.

Issue:- In getManager() sometimes neo4j db throws an error while running a query so control ends up in catch block. However, somehow promise will not be resolved after that. So I am not sure if I need to call deferred.reject(returnResults) in catch block.

1) BOT dialog Consumer: calling getPersonInfo()

helper.getPersonInfo(personFullName)
    .then(function(results) {
        if (results && results.length >= 1) {
            //Do something.
        } else {
            //Do something.
        }
    })
    .catch(function(error) {
        //Do something
    });

2) how getPersonInfo() looks:

getPersonInfo: function(fullname) {
    return Promise.all([
                    personService.getManager(firstname, fullname, operatorId),
                    personService.getTeamsMates(firstname, fullname, operatorId)
                ]);
}

3) How one of the method on promise.all() looks: -

var Q = require('q')

getManager: function(fullname) {
    let session = graphDBDriver.session();
    let deferred = Q.defer();
    let query = function() {
        let returnResults = [];
        if (fullname) {

            let cypherQuery = "Neo4j Query"

            session
                .run(cypherQuery, { fullname: fullname })
                .then(function(result) {
                    result.records.forEach(function(record) {
                        if (record && record.length >= 1) {
                            returnResults.push(record);
                        }
                    });

                    return deferred.resolve(returnResults);

                    session.close();

                })
                .catch(function(error) {
                    session.close();
                    console.log(" Neo4j error from getManager: " + error);


                });
        } else {
            return deferred.reject(returnResults);
        }
    }
    query();
    return deferred.promise;
}

Questions:-

1) Is it good practice to deferred.reject(returnResults) in catch block of getManager()?

2)Any other pattern or code changes I should do as per best practices?.

In the catch from neo, just repeat your reject passing 'error.message' — Robert Rowntree, Jun 08 '18 at 19:56
This is https://github.com/petkaantonov/bluebird/wiki/Promise-anti-patterns#the-deferred-anti-pattern — Estus Flask, Jun 08 '18 at 20:10

score 2 · Answer 1 · answered Jun 08 '18 at 20:28

Is it good practice to deferred.reject(returnResults) in catch block of getManager()?

No, never reject anything that is not about the reason of rejection.

Any other pattern or code changes I should do as per best practices?.
- In session.then(...), session.close(); will never execute after the return statement.
- You don't have to embed the session.run(...) logic in query(). Also, if session.run().then().catch() will return a Promise, you can just return it directly instead of initializing a new promise and resolving/rejecting explicitly.
- error in session.catch(...) should be handled by deferred.reject as well instead of just logging it to console.
- If you are not using the ancient Node.js, it should have native promise, you don't have to use "Q".

This is how I would implement getManager:

getManager : function (fullname) {
    // ideally, `fullname` should be checked before calling this function
    // if this function is only for private use and totally controllable.
    if (fullname) {
        // only initialize variables when necessary
        const returnResults = [];
        const session = graphDBDriver.session();
        const cypherQuery = "Neo4j Query";
        return session
            .run(cypherQuery, {
                fullname: fullname
            })
            .then(function(result) {
                result.records.forEach(function(record) {
                    if (record && record.length >= 1) {
                        returnResults.push(record);
                    }
                });
                session.close();
                // `return value` in `.then()` is similar to `resolve(value)`
                return returnResults;
            })
            .catch(function(error) {
                session.close();
                console.log(" Neo4j error from getManager: " + error);
                // `throw value` in `.catch()` is similar to `reject(value)`
                // throw it so that it can be caught
                throw error;
            });
    } else {
        const error = new Error('`fullname` is required');
        // always return a promise
        return Promise.reject(error);
    }
}

Thanks for the detailed explanation @Kitce. About module 'Q' are you saying I don't need to use let deferred = Q.defer(); I thought using deferred.promise has some advantages. — Royjad, Jun 08 '18 at 23:50
Yes, it is unnecessary to initialize a new Promise when your logic already returns a Promise. Also, you don't need Q unless you need something does not exist in native Promise. For example, I use "bluebird" for Promise, because I usually use its `Promise.props`, `Promise.map` and etc. — kit, Jun 09 '18 at 06:52
Even then, `Q.defer` is antipattern. If native or other promise needs to be converted to Q, it's `Q(promise)` or `Q.when(promise)`. As something that doesn't exist in native promise, there are [ponyfills](https://github.com/sindresorhus/promise-fun#packages), they can be used in functional manner with native promises (there's a good reason to prefer them since the introduction of async..await). — Estus Flask, Jun 09 '18 at 10:49

Best way to handle the nodejs promise

1 Answers1