0

I have the following JavaScript promise chain. It works as expected.

signUp (data) {
  return oneFunction(username).then((usernameExist) => {
    return firebaseAuth.createUserWithEmailAndPassword(data.email, data.password).then((user) => {
      firebaseDb.ref('users/' + user.uid + '/public/').set(userData).then()
      utils.updateUsernameMapping(data.username, user.uid).then()
      return user.updateProfile({
        displayName: data.displayName
      }).then(function () {
        return user
      }, error => {
         throw error
      })
    })
  }).catch(error => {
    throw error
  })
}

However, I believe the signUp function is hard to decipher because of the nested levels. I tried to change it to the following approach:

userPromise
.then()
.then()
.then();

But I couldn't get it working because the user variable needs to be passed down the chain. Ideally, I would like to minimize this code for readability and use one catch() for efficiency. Any ideas appreciated.

UPDATE: Following feedback from Bergi, below is my updated code:

signUp (email, password, displayName, username) {
  const userData = { username: username, lastLogin: Firebase.database.ServerValue.TIMESTAMP }
  return utils.checkIfUserExists(username).then(usernameExist => {
    return firebaseAuth.createUserWithEmailAndPassword(email, password)
  }).then(user => {
    return Promise.all([
      firebaseDb.ref('users/' + user.uid + '/public/').set(userData),
      utils.updateUsernameMapping(username, user.uid),
      user.updateProfile({displayName})
    ]).then(() => user)
  })
},
londonfed
  • 1,170
  • 2
  • 12
  • 27
  • I don't think `error => { return error }` works as you'd expect. And why are you calling `.then();` without arguments on `firebase.ref…` and `utils.updateUsernameMapping…`?! – Bergi Jul 28 '17 at 16:44
  • Just declare a var higher up the scope chain and assign it on the first call. Or if your a stickler, return it in an array with the other value and use destructuring to access it. – Jared Smith Jul 28 '17 at 16:44
  • Please check [this answer](https://stackoverflow.com/a/35805818/7564182) for clarification on promise chaining... – Myonara Jul 28 '17 at 16:45
  • 1
    @JaredSmith No, don't do that. – Bergi Jul 28 '17 at 16:46
  • Thanks @Bergi I edited the post. – londonfed Jul 28 '17 at 16:47

2 Answers2

2

Error handlers that just rethrow the error are pointless, omit them.

You can unnest the outermost level with the usernameExist variable that you don't need anywhere else:

signUp (data) {
  return oneFunction(username).then(usernameExist => {
    return firebaseAuth.createUserWithEmailAndPassword(email, password);
  }).then(user => {
    return Promise.all([
      firebaseDb.ref('users/' + user.uid + '/public/').set(userData),
      utils.updateUsernameMapping(username, user.uid),
      user.updateProfile({displayName})
    ]).then(() => user);
  });
}

There's nothing wrong with the nested then that ensures that user is returned in the end. There are a few approaches to tackle this problem, nesting closures is just fine.

Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • Thanks for a great answer. This is exactly what I was looking for as looks a lot neater. I'll repost your answer with a few minor modifications. – londonfed Jul 28 '17 at 17:00
0

For multiple promises use

p1 = new Promise(); p2 = new Promise(); p3 = new Promise(); Promise.all([p1, p2, p3])

Promise.all documentation

dfee
  • 86
  • 1
  • 4