1

I have a function to log in a user which should return JSON.

const username = req.body.username;
const password = req.body.password;

if (!username) {
  throw new Error('Missing username');
}

if (!password) {
  throw new Error('Missing password');
}

User.findOne({ username, password }).then(user => {
  res.json({ user });
}).catch(err => {
  res.json({ err });
});

but then the errors for missing username or missing password are not returned in JSON.

I could change it to

const username = req.body.username;
const password = req.body.password;

if (!username) {
  res.json({ err: 'Missing username' });
}

if (!password) {
  res.json({ err: 'Missing password' });
}

User.findOne({ username, password }).then(user => {
  res.json({ user });
}).catch(err => {
  res.json({ err });
});

but it seems a little redundant.

Is the correct way to do it to encapsulate it in a promise?

Jamgreen
  • 10,329
  • 29
  • 113
  • 224

4 Answers4

2

In your first solution, the thrown errors won't be handled, because you throw them outside of promise chain and without try/catch block. In your second solution you can get cannot send headers after they sent error, because the response can be sent twice (username is missing and password is missing).

So the one of the possible solutions here, is to create a promise chain (using Promise.resolve()) and validate parameters here:

function validateParams() {
  const username = req.body.username;
  const password = req.body.password;

  if (!username) {
    throw new Error('Missing username');
  }
  if (!password) {
    throw new Error('Missing password');
  }
  return { username, password };
}

Promise
  .resolve()
  .then(validateParams)
  .then(filter => User.findOne(filter))
  .then(user => res.json(user))
  .catch(err => res.json(err));
alexmac
  • 19,087
  • 7
  • 58
  • 69
0

The obvious way would indeed be to encapsulate them in a promise to start your promise chain (with the User.findOne being inside the first then-block) - that way your current error handler catches them just fine.

jlaitio
  • 1,878
  • 12
  • 13
0

you can wrap your functions in a promise and handle it efficiently

function getRes(){  
    return new Promise(function(resolve, reject){
        const username = req.body.username;
        const password = req.body.password;

        if (!username) {
          reject(new Error('Missing username'));
        }

        if (!password) {
          reject(new Error('Missing password'));
        }

        resolve(User.findOne({ username, password }));
    });
}


getRes().then(function(result){
    res.json(result);
}).catch(function(err){
    res.json(err);
})
marvel308
  • 10,288
  • 1
  • 21
  • 32
  • Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! – Bergi Sep 11 '17 at 09:59
  • If you insist on using the `new Promise` constructor, either put the `User.findOne` promise in a `then` handler after it, or call `resolve(User.findOne(…))` at the end. – Bergi Sep 11 '17 at 10:00
0

I'm taking the example from @alexmac and use es6 async feature:

function validateParams() {
  const username = req.body.username;
  const password = req.body.password;

  if (!username) {
    throw new Error('Missing username');
  }
  if (!password) {
    throw new Error('Missing password');
  }
  return { username, password };
}

async function resolver() {
    try {
        await resolve()
        let filter = validateParams()
        let user = await User.findOne(filter)
        await res.json(user)
    } catch (e) {
        await res.json(e)
    }
 }

and that would look more elegant by using an if instead of a throw:

async function(req, res) {
    const password = req.body.password
    const username = req.body.username
    let c = !password ? 'missing password' : 
                !username ? 'missing username' : null
    if (!c) {
       c = await User.findOne({ username, password })
    }
    await res.json(c)
}
CFrei
  • 3,552
  • 1
  • 15
  • 29