Promise Structure - Am I doing this right?

Question

I'm currently building out a node.js project to load a user profile based on a call made from my middleware. At the moment I'm just starting out on using promises and I've now hit the nested promise world but I feel as though I am implementing them as I would have done with the callback hell structure.

My middleware:

const { mount } = require('telegraf');
const log = require('../Utilities/Log')
const Authenticator = require('../User/Authenticator')

/* eslint no-param-reassign: ["error", { "props": false }] */
module.exports = () => mount('text', (ctx, next) => {
  // Attempt to login the user given their ID
  Authenticator.login(ctx.from.id, ctx.from).then((userObject) => {
    // Returned user is attached to the context state to pass through middleware
    ctx.state.user = userObject;
    log.debug(ctx.state.user)
    // Continue with the attached user object through rest of middleware
    return next();
  }).catch((err) => {
    // Do not send a next() as we have hit an error.
    return log.error(err)

  })
});

Authenticator with login method:

const User = require('../User/User')

class Authenticator {
    static login(userID = 0, userFrom = {}) {
        return new Promise ((resolve, reject) => {
            User.getUserByID(userFrom.id).then((userRecord) => {
                let result = {}
                if (!userRecord) {
                    result = new User('', userFrom.first_name, userFrom.username, '', userID)
                } else {

                    result = new User(userRecord._pNick, userRecord._firstName, userRecord._userName, userRecord._phoneNumber, userID)
                }             
                return resolve(result)
            }).catch((err) => {
                return reject(err)
            })
        })
    }
}

My User class with method to retrieve from DB

const dbConnection = require('../Services/db/Db')
const config = require('../Config')
const USER_COLLECTION = config.databaseConnection.DB_USER_COLLECTION

// Set database connection to use the User table
const userTable = dbConnection.collection(USER_COLLECTION)

class User {
    constructor(pNick = '', firstName = '', userName = '', phoneNumber = '', userIdentifier = 0) {
        this._pNick = pNick
        this._firstName = firstName
        this._userName = userName
        this._phoneNumber = phoneNumber
        this._userID = userIdentifier
        this._isAdmin = this.isAdmin()
    }

    static getUserByID(userID = 0) {
        return new Promise( (resolve, reject) => {
            userTable.findOne({
            _userID: userID
        }, (err, user) => {
            if (err) {
                return reject(err)
            }
            return resolve(user)
        })
    })
    }

    isAdmin () {
        if (this._userID === config.adminUser.ADMIN_ID) {
            return true
        }
        return false
    }

    isAuthenticated () {
        if (this._pNick.length() > 1) {
            return true
        }
        return false
    }

}

module.exports = User

It feels as though I could be flattening this structure somewhat?

*"It feels as though I could be flattening this structure somewhat?"* Yes indeed, see the linked question's answers. When you *already* have a promise (such as the one from `User.getUserByID` in `Authenticator#login`), you don't want or need `new Promise`; `then` and `catch` return promises, so you just chain onto the one you already have. — T.J. Crowder, Nov 11 '17 at 10:14
Sometimes people take having their question closed as a duplicate as a judgement they shouldn't have asked it. That's not always the case, duplicates can be very useful, increasing the search surface for future visitors. Closing as duplicate just helps the site have canonical *answers* rather than duplicating them everywhere. — T.J. Crowder, Nov 11 '17 at 10:15
Thanks for this - I've reviewed and still can't get my head around it. When you say "just chain onto the one you already have" what does this mean? For now I've resolved the issue by removing my call in to Authenticator#login and I've brough the User.getUserByID into my Middleware. This however is just dodging the problem I think. — munkee, Nov 11 '17 at 17:19
For instance, `Authenticator` would look like this: https://pastebin.com/Fb7QNKhD Note that we *return* the result of calling `then`, which creates a new promise; that promise will be fulfilled with whatever we return in the `then` callback. Also note that I removed the `catch` handler. One of the rules of promises: Either return the promise, or catch errors, but almost never *both*. Leave it so the caller can catch it. — T.J. Crowder, Nov 11 '17 at 17:26
(I just noticed `let result = {}` followed by an `if`/`else` where both branches write to `result`. You don't want that initializer there then. Just `let result;`.) — T.J. Crowder, Nov 11 '17 at 17:27
Thanks your example makes much more sense now. I've ended up with this https://gist.github.com/ImTheDeveloper/978e24d1fbedb1d7d1510e2380cdf914 — munkee, Nov 11 '17 at 19:36

Promise Structure - Am I doing this right?

0 Answers0