I have this code, with a lot of blocks of returns, by example SignUp()
connectors.js
const connectors = {
Auth: {
signUp(args) {
return new Promise((resolve, reject) => {
// Validate the data
if (!args.email) {
return reject({
code: 'email.empty',
message: 'Email is empty.'
});
} else if (!isEmail(args.email)) {
return reject({
code: 'email.invalid',
message: 'You have to provide a valid email.'
});
}
if (!args.password) {
return reject({
code: 'password.empty',
message: 'You have to provide a password.'
});
}
return encryptPassword(args.password, (err, hash) => {
if (err) {
return reject(new Error('The password could not be hashed.'));
}
return User.create(Object.assign(args, { password: hash }))
.then((user) => {
resolve(createToken({ id: user._id, email: user.email }));
})
.catch((err2) => {
if (err2.code === 11000) {
return reject({
code: 'user.exists',
message: 'There is already a user with this email.'
});
}
return reject(err2);
});
});
});
},
};
module.exports = connectors;
then anoter code that call this code:
const connectors = require('./connectors');
CallsignUp(root, args) {
const errors = [];
return connectors.Auth.signUp(args)
.then(token => ({
token,
errors
}))
.catch((err) => {
if (err.code && err.message) {
errors.push({
key: err.code,
value: err.message
});
return { token: null, errors };
}
throw new Error(err);
});
}
how it's possible to avoid this in ES6 or ES7 or ES2017?
there are:
return()
.then()
return()
.then
and just loop returns:
return()
return()
return()
comming from PHP this code looks crazy, because return functions that return functions, and it's not clear for me, how is the name in javascript this type of block return code? calling functions that returns again more code?
UPDATED:
Code is not mine, full source in
https://github.com/jferrettiboke/react-auth-app-example
I'd like to understand by example:
return encryptPassword(args.password, (err, hash) => {
if (err) {
return reject(new Error('The password could not be hashed.'));
}
return User.create(Object.assign(args, { password: hash }))
.then((user) => {
.catch((err2) => {
return reject(err2);
/src/utils/auth.js (here is encryptPassword)
const jwt = require('jsonwebtoken');
const bcrypt = require('bcrypt-nodejs');
const config = require('../config');
exports.encryptPassword = (password, callback) => {
// Generate a salt then run callback
bcrypt.genSalt(10, (err, salt) => {
if (err) { return callback(err); }
// Hash (encrypt) our password using the salt
return bcrypt.hash(password, salt, null, (err2, hash) => {
if (err2) { return callback(err2); }
return callback(null, hash);
});
});
};
there are 3 returns calling functions and returning values and functions? OOP is never like this, how to use Async/Await suggest by @dashmud, i don't want to learn old things like callbacks
Extending @jfriend00's answer, here's an approach that uses the ES2017 async / await syntax to flatten the callback pyramid or callback hell, however you prefer to call it:
const encryptPasswordPromise = require('util').promisify(encryptPassword)
const connectors = {
Auth: {
async signUp (args) {
const { email, password } = args
// Validate the data
let err
if (!email) {
err = { code: 'email.empty', message: 'Email is empty.' }
} else if (!isEmail(email)) {
err = { code: 'email.invalid', message: 'You have to provide a valid email.' }
} else if (!password) {
err = { code: 'password.empty', message: 'You have to provide a password.' }
}
if (err) {
throw err
}
let hash
try {
hash = await encryptPasswordPromise(password)
} catch (err) {
throw new Error('The password could not be hashed.')
}
const { _id: id, email } = await User.create(Object.assign(args, { password: hash }))
try {
return createToken({ id, email })
} catch (err) {
if (err.code === 11000) {
throw { code: 'user.exists', message: 'There is already a user with this email.' }
} else {
throw err
}
}
}
}
}
module.exports = connectors
Rather than handwrite my own promisified promise-based encryptPassword(), I opted to use a node.js builtin transformation function for that called util.promisify().
Overall, there are still a few improvements that could be made like moving the validation of the signUp() arguments to a separate function, but none of that is related to flattening the "callback hell" anti-pattern that gave rise to promise-based control-flow and async/await syntax.
This code needs to be refactored in a number of ways. First, you really, really don't want to mix promises and plain async callbacks in the same logic flow. It makes a mess and wrecks a lot of the advantages of promises. Then, you have an anti-pattern going using promises inside of new Promise(). Then, you have more nesting that is required (you can chain instead).
Here's what I'd suggest:
function encryptPasswordPromise(pwd) {
return new Promise((resolve, reject) => {
encryptPassword(pwd, (err, hash) => {
err ? reject(new Error("The password could not be hashed.")) : resolve(hash);
});
});
}
const connectors = {
Auth: {
signUp(args) {
// Validate the data
let err;
if (!args.email) {
err = {code: 'email.empty', message: 'Email is empty.'};
} else if (!isEmail(args.email)) {
err = {code: 'email.invalid', message: 'You have to provide a valid email.'};
} else if (!args.password) {
err = {code: 'password.empty', message: 'You have to provide a password.'};
}
if (err) {
return Promise.reject(err);
} else {
return encryptPasswordPromise(args.password).then(hash => {
args.password = hash;
return User.create(args);
}).then((user) => {
return createToken({id: user._id, email: user.email});
}).catch(err2 => {
if (err2.code === 11000) {
throw new Error({code: 'user.exists', message: 'There is already a user with this email.'});
} else {
throw err2;
}
});
}
}
}
};
Summary of Changes:
- Collect all errors initially and do return
Promise.reject(err) in one place for all those initial errors.
- Promisify
encryptPassword() so you aren't mixing regular callbacks with promise logic flow and you can then properly return and propagate errors.
- Removing wrapping of the whole code with
return new Promise() since all your async operations are now promisified so you can just return promises directly. This really helps with proper error handling too.
- Undo unneccessary nesting and just chain instead.
- Remove
Object.assign() as there did not appear to be a reason for it.
In your edit, you asked for an explanation of this code segment:
return encryptPassword(args.password, (err, hash) => {
if (err) {
return reject(new Error('The password could not be hashed.'));
}
return User.create(Object.assign(args, { password: hash }))
.then((user) => {
.catch((err2) => {
return reject(err2);
- This code calls
encryptPassword().
- It returns the result of that which is probably
undefined so all the return is doing is controlling program flow (exiting the containing function), but since there is no code after that return at the same level, that's unnecessary.
- You pass a callback to
encryptPassword(). That callback is asynchronous, meaning it is called some time later.
- The callback gets two arguments:
err and hash. In the node.js async calling convention, if the first argument is truthy (e.g. contains some truthy value), then it represents an error. If it is falsey (usually null), then there is no error and the second argument contains the result of the asynchronous operation.
- If there was an error, the parent promise is rejected and the callback is exited. Again, there is no return value from
reject so the return is just for exiting the callback at that point (so no other code in the callback runs).
- Then, another asynchronous operation
User.create() is called and it is passed the args object with a new password set in it.
User.create() returns a promise so its result is captured with a .then() method and a callback passed to that.- Some time later, when the asynchronous
User.create() finishes, it will resolve its promise and that will cause the .then() callback to get called and the result will be passed to it. If there's an error in that operation, then the .then() callback will not be called, instead the .catch() callback will be called. In that .catch() callback, the parent promise is rejected. This is a promise anti-pattern (resolving a parent promise inside another promise) because it is very easy to make mistakes in proper error handling. My answer shows how to avoid that.
First things first. There are no loops in your code.
If you're coming from PHP, then I would guess Javascript's asynchronous execution model looks alien to you. I would suggest learning more about Javascript.
Learn the following in the following order:
- Callbacks
- Promises
- Generators
- Async/Await
Those are the different approaches on how to handle Javascript's asynchronous execution model starting from the most basic (callbacks) to the most modern approach, "async/await".
Async/await would be the most readable but it would be better if you understand how async/await came to be since they are easy to use the wrong way if you don't understand what you're doing.
