I'm new to Bluebird and I'm trying to create a new User but the reject function doesn't work as expected.
The problem is that it creates me the user even though it launches the error There nickname is already in use.
Here bellow I paste my code.
User.js
var User = require('../models/user');
var Promise = require('bluebird');
module.exports = {
validateFields: function (nickname) {
return new Promise(function (response, reject) {
if (!nickname) {
reject('You must provide the nickname');
} else if (nickname.length < 4 || nickname.length > 20) {
reject('The nickname must be longer than 4 and shorter than 20 characters');
} else {
nickname = nickname.trim();
User.findOne({ "nickname": nickname }).exec()
.then(function (user) {
if (user) {
reject('There nickname is already in use');
} else {
response();
}
}, function (err) {
reject('There is an error trying to verify the nickname');
});
}
});
},
registerUser: function (user_id, nickname) {
return new User({ user_id: user_id, nickname: nickname }).save();
}
};
register.js
var validator = require('validator');
var Promise = require('bluebird');
var Account = require('../../models/account');
module.exports = {
validateFields: function (email, password) {
return new Promise(function (response, reject) {
if (!email) {
reject('You must provide the email');
} else if (!password) {
reject('You must provide the password');
} else if (email.length < 6) {
reject('The email is too short');
} else if (email.length > 40) {
reject('The email is too long');
} else if (!validator.isEmail(email)) {
reject('The email is not valid');
} else {
Account.findOne({ email: email }).exec()
.then(function (account) {
if (account) {
reject('There is already an email');
} else {
console.log(account);
response();
}
}, function (err) {
reject('There is an error trying to verify the email');
});
}
});
},
registerAccount: function (email, password) {
return new Account({ email: email, password: password }).save();
}
};
routes.js
var Promise = require('bluebird');
var user = require('./routes/user');
var account = require('./routes/auth/register');
router.post('/register', function (req, res, next) {
account.validateFields(req.body.email, req.body.password)
.then(function () {
return user.validateFields(req.body.nickname);
}, function (err) {
return res.status(400).json({ msg: err });
})
.then(function () {
req.body.email = req.body.email.trim();
req.body.password = req.body.password.trim();
console.log('bien');
return account.registerAccount(req.body.email, req.body.password);
}, function (err) {
console.log('mal');
return res.status(400).json({ msg: 'There was an error trying to save the email' });
})
.then(function (data) {
return user.registerUser(data, req.body.nickname);
}, function (err) {
return res.status(400).json({ msg: 'There was an error trying to save the user' });
})
.then(function () {
return res.status(200);
}, function (err) {
console.log(err);
return res.status(400).json({ msg: err });
})
.catch(function (err) {
console.log('catch');
return res.status(400).json({ msg: err });
});
});
Thanks in advice.
UPDATE
Just to clarify everybody that start with promises and is looking for best practices, I guess this link is helpful.
TLDR; don't catch errors with .catch() or .then(success , error) if you don't want the subsequent functions in the promise chain to be called after an error. Catch only at the end of the chain to get the result of the whole async calls chain without having undesired calls after errors.
Okay, let's imagine a function that just returns a rejected promise:
function fakeForbiddenAsyncOperation(){
return new Promise(function(resolve , reject){
return reject('This operation is forbidden');
});
}
Then, a Promise chain like:
fakeForbiddenAsyncOperation().then(
function(){
console.log('first parameter, success');
},
function(err ){
console.log('second parameter, failure: ' + err);
})
.then(function(){
console.log('This log is called, because the previous error was catched in the second then() lambda');
})
.catch(console.log);
Will let that console.log 'this log is called...' to run, because the error is being handled. The output will be:
second parameter, failure: This operation is forbidden
This log is called, because the previous error was catched in the second then() lambda
What you want to do in your code, preventing the User to being created if there is a previous error in the validation, is more like:
fakeForbiddenAsyncOperation().then(
function(){
console.log('first parameter, success');
})
.then(function(){
console.log('This log is called');
} , function(err){
console.log('There was an err: ' + err);
console.log('this is called at the end, and the previous "this log is called" log wasn\'t fired because there was an unhandled rejection');
});
Which output is:
There was an err: Error: This operation is forbidden
this is called at the end, and the previous "this log is called" log wasnt fired because there was an unhandled rejection
So the 'first parameter, success' log will never fire (so, a User.create() function, or any other operation you don't want to perform if there is a previous error, would't too).
There are also 2 minor issues you maybe want to handle:
Bluebird documentation recommends to use .catch() instead of .then(success, failure):
forbiddenAsyncOperation().then(
function(){
console.log('first parameter, success');
})
.then(function(){
console.log('This log is called');
})
.catch(function(){
console.log('this is called at the end, and the previous "this log is called" log wasn\'t fired because there was an unhandled rejection');
});
Will behave like the previous example.
Also, is better to reject errors instead of strings:
reject(new Error('The nickname must be longer than 4 and shorter than 20 characters'));
Will print error stack traces instead of just a message in the console.