Logging a user in and creating a session

I have a bunch of functions that return promises and need to chain them together. However, it's currently difficult to read and I would expect it to be even harder to maintain.

I have a function that performs the following actions:

• Logs off any inactive users.

• Verifies a user's credentials.

• Creates a session and logs the user in.

Is there a better way?

generateToken: function (email, password) {
    return new Promise((resolve, reject) => {
        //Kill inactive sessions first
        module.exports.destroyInactive()
        .then(() => {
            //Verify that the password matches the user
            users.checkPassword(email, password, sharedSecret)
            .then((user) => {
                //Check if there is another user already signed in, if there is then we can't
                //produce another token until they are signed out.
                signedon.getByEmailAddressThisProcess(email)
                .then((result) => {
                    let rows = result[0];

                    if (rows.length > 0)
                        return reject('A user with email: ' + email + ' is already logged in.');

                    //Get the current XPROCESS.
                    xprocess.getByProcessId(process.pid)
                    .then((xproc) => {
                        //Create a brand new session object
                        let session = generateSession(user);

                        //Save the session in the signed on table.
                        signedon.signin(session, xproc.pid)
                        .then(() => {
                            resolve(parseSessionToToken(session));
                        },
                        reject);
                    },
                    reject);
                },
                reject);
            },
            reject);
        },
        reject);
    });
},

Potential improvements

generateToken(email, password) {
  let user, session;

  return module.exports.destroyInactive()
    .then(() => users.checkPassword(email, password, sharedSecret))
    .then(_user => user = _user)
    .then(() => signedon.getByEmailAddressThisProcess(email))
    .then(([rows]) => {
      if (rows.length > 0) {
        return Promise.reject(`A user with email: ${email} is already logged in`);
      }

      return xprocess.getByProcessId(process.pid);
    }).then(xproc => {
      session = generateSession(user);

      return signedon.signin(session, xproc.pid);
    }).then(() => parseSessionToToken(session));
}

Your method falls into the nested promises antipattern, very similar to the classic callback pyramid of doom.

This solution relies on chaining promises. Since we need the variables user and session in non-adjacent promises we need to have references of them outside of the promise chain.

Since you are already using Promises which are an ES6 feature, you can take advantage of extra ES6 goodness. See es6-features.org for more.

ES6 features

Enhanced object method properties

generateToken: function (email, password) can be simplified by using to generateToken(email, password)

Arrow Function Expression Bodies

users.checkPassword(...) is implicitly returned. Since this is a Promise, the chain won't continue to be executed until the returned Promise is resolved/rejected. This means we can just return the promise and not have to continue to nest using checkPassword(...).then(...)

Array Matching

let rows = result[0] can be simplified to rows = [result], which can then be used as the resolve parameter (([rows]) => ...)

Promise.reject

Returning Promise.reject allows us to reject the Promise without nesting.

Template Literals/String Interpolation

Provide a (subjectively) more readable solution by not having to concatenate strings with variables.

'A user with email: ' + email + ' is already logged in.'

becomes

`A user with email: ${email} is already logged in`

Arrow Function Statement Bodies

Since we are doing multiple things inside the rows Promise, we use => statement bodies.

Due to this we need to explicitly return the Promise (so that the chain doesn't continue to execute until the Promise is resolved/rejected). Otherwise undefined will be resolved, not waiting for the Promise.

Note:

Nowhere are you catching rejected promises using catch, are you catching a rejected generateToken attempt?

Promises can greatly improve readability and remove nested callbacks when used correctly, but it is important to not allow them to "swallow" errors. At the very least make sure to log rejected Promises. A simple somePromise(...).catch(console.error) is better than nothing. Prefer console.error over console.log when possible since it's more "noisy".

StackExchange Code Review Q#154056, answer score: 4