CODEWITHSUNDEEP
javascripttypescriptpromise
gremo
gremo

Reputation: 48899

Understanding promise chaining with this simple example

I need the following logic but I can't get it. When user is found (not undefined) I need to compare the password (another promise) which returns a boolean). Need to:

First attempt (ugly, unreadable):

  @Post()
  login(
    @BodyParam('username', { required: true }) username: string,
    @BodyParam('password', { required: true }) plainPassword: string,
  ) {

    return this.userRepository.findOne({ username: username, enable: true })
      .then ((user: User | undefined) => {
        if (!user) {
          return undefined; // 404
        }

        return bcrypt.compare(plainPassword, user.password)
          .then(passwordMatch => {
            if (!passwordMatch) {
              throw new ForbiddenError('Authentication failed.'); // 403
            }

            return user; // 200
          });
      });
  }

Second attempt doesn't work, always returns 'ok':

return this.userRepository.findOne({ username: username, enable: true })
  .then((user: User | undefined) => {
    if (!user) {
      return undefined; // 404
    }

    return bcrypt.compare(password, user.password);
  })
  .then(passwordMatch => {
    // Doesn't work: this is executed every time (even if user is undefined).

    return 'ok';
  });

Upvotes: 0

Views: 84

Answers (2)

T.J. Crowder
T.J. Crowder

Reputation: 1074148

Your then handler at the end is always run (well, if the promises don't reject) because the first promise resolves with undefined if the user doesn't exist, or with a boolean if the user does exist.

Your nested version was fine. If you need to return user in the successful case, it's probably the way to go.

But if you just need to return 'ok' as in your second code example in the successful case, you could flatten things, you just have to handle the undefined you'll get if there's no user. We can also take advantage of the fact that you know user will have the value undefined if the user wasn't found:

return this.userRepository.findOne({ username: username, enable: true })
  // The `then` below returns `undefined` if `user` is `undefined`, the promise from `compare` otherwise
  .then((user: User | undefined) => user && bcrypt.compare(password, user.password))
  .then(passwordMatch => {
    if (passwordMatch === undefined) {
      // No user
      return undefined;
    } else if (!passwordMatch) {
      // Bad password
      throw new ForbiddenError('Authentication failed.'); // 403
    } else {
      // All good
      return 'ok';
    }
  });

If you want to flatten it and return user, then you need to propagate user to the next handler:

return this.userRepository.findOne({ username: username, enable: true })
  .then((user: User | undefined) => {
    return !user
        ? {user} // will be {undefined}
        : bcrypt.compare(password, user.password)
            .then(passwordMatch => ({user, passwordMatch})); // *** Note making an object
  })
  .then(({user, passwordMatch}) => { // *** Note destructuring
    if (user === undefined) {
      // No user
      return undefined;
    } else if (!passwordMatch) {
      // Bad password
      throw new ForbiddenError('Authentication failed.'); // 403
    } else {
      // All good
      return user; // 200
    }
  });

(That first then handler could be a concise arrow as in the first code block above, but it started getting ugly.)

Upvotes: 3

SLaks
SLaks

Reputation: 887305

When you return undefined, that makes the first then() callback's promise resolve to undefined.

Your second then() callback then executes, receiving undefined as a parameter.

You should change the second callback to check for that.

Upvotes: 0

Related Questions