Is it good to use Promise this way?

Question

I'm trying to understand and master promise but I ran into an issue which I'm not sure if this would be the right way to do things.

I'm using nodejs and I have the following: A database and a module in which I do something with the database. At one point I'm trying to do create a new user in the database and after the user is created to create another entry in another table for that user with other details.

passport.use('local-signup', new LocalStrategy({
    usernameField: 'username',
    passwordField: 'password',

  },
  function(username, password, done) {

    // Before creating the user access Sequlize beforeCreate method so we can encrypt the password
    User.beforeCreate(function(req) {
      console.log('Encryptying password for user ',req.username)
      return encryptPass(req.password)
      .then(success => {
        req.password = success;
      })
      .catch(err => {
        if (err) console.error(err);
      });
    });

    User.create({
      username: username,
      password: password
    }).then(function(createdUser) {
      console.log(createdUser);
      UserDetails.create({
        id:createdUser.dataValues.id
      }).then((data)=>{
        console.log(data);
        return done(null,createdUser);
      }).catch((error)=>{
        console.log(error)
        return done(error)
      })
    })
    .catch(function(error) {
      console.error(error)
      return done(`${error.message}.`);
    });
  }
));

Is this the correct way to use the promises when I have something like this?

If you need more information please let me know and I'll try to make everything more clearer as much as I can.

Best Regards, Victor

I don't think there is a right way or wrong way of using Promises in JavaScript — Felix Fong, Mar 08 '18 at 11:19
Can you please provide the function encapsulating here. Presumably the function which has the argument `done`. Also, is `done` acting like a Promise `resolve`? — wmash, Mar 08 '18 at 11:21
@FelixFong That's not true. There are myriads of `Promise` anti-patterns that make code error-prone and hard to follow. — nicholaswmin, Mar 08 '18 at 11:23
Possible duplicate of [JS ES6 Promise Chaining](https://stackoverflow.com/questions/35711084/js-es6-promise-chaining) — nicholaswmin, Mar 08 '18 at 11:25
@wmash I have added the full code in the description at the end. I'm using a passport module for a sing-up process. — Victor, Mar 08 '18 at 11:27
@Nicholas Kyriakides As long as you are aware of don't nested too much and turns out to be a callback hell, or maybe start using the new `async/await` function, I'm sure your code will look beautiful and readable, for more information `async/await` you can visit this website at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function — Felix Fong, Mar 08 '18 at 11:28
@NicholasKyriakides Thanks for mentioning that, i'll take a look. — Victor, Mar 08 '18 at 11:28
@FelixFong I should take a look and try to learn async/await :) I've just moved form callbacks to promises as I'm still a junior but async/await looks like a new good thing. thanks. — Victor, Mar 08 '18 at 11:47

score 1 · Answer 1 · answered Mar 08 '18 at 11:30

You can simplify this a little, because you can remove inner catch block, just need to return inner Promise

User.create({
    username: username,
    password: password
}).then(function (createdUser) {
    console.log(createdUser);
    return UserDetails.create({
        id: createdUser.dataValues.id
    }).then((data) => {
        console.log(data);
        return done(null, createdUser);
    })
}).catch(function (error) {
    console.error(error);
    return done(`${error.message}.`);
});

Rest looks OK

score 0 · Accepted Answer · answered Mar 08 '18 at 11:43

Thank you all, based on comments I have ended up with the below code. As much as I tried to avoid callback hell issue it still looks a bit like it but since both promises needs to be called I think it's a good solution and not nesting it to much:

 // Local sign-up strategy
  passport.use('local-signup', new LocalStrategy({
    usernameField: 'username',
    passwordField: 'password',

  },
  function(username, password, done) {

    // Before creating the user access Sequlize beforeCreate method so we can encrypt the password
    User.beforeCreate(function(req) {
      console.log('Encryptying password for user ',req.username)
      return encryptPass(req.password)
      .then(success => {
        req.password = success;
      })
      .catch(err => {
        if (err) console.error(err);
      });
    });

    User.create({
      username: username,
      password: password
    }).then(function(createdUser) {
      console.log(createdUser);
      return createdUser;
    }).then((userData) =>{
      UserDetails.create({
        id:userData.dataValues.id
      }).then((userDetails)=>{
        return done(null,userData)
      })
    }).catch(function(error) {
      console.error(error)
      return done(`${error.message}.`);
    });
  }
));

Thank you, Victor

Is it good to use Promise this way?

2 Answers2