0

trying to change .then, like this:

User.prototype.login = () => {
  return new Promise((resolve, reject) => {
    this.cleanup();
    usersCollection
      .findOne({ username: this.data.username })
      .then((attemptedUser) => {
        if (attemptedUser && attemptedUser.password == this.data.password) {
          resolve("logged in");
        } else {
          reject("invalid something");
        }
      })
      .catch(() => {
        reject("Please, try again later");
      });
  });

First one works perfectly, but when I try to change it to async/await, like this:

User.prototype.login = () => {
  return new Promise(async (resolve, reject) => {
    this.cleanup();
    try {
      const attemptedUser = await usersCollection.findOne({ username: this.data.username });
      if (attemptedUser && attemptedUser.password == this.data.password) {
        resolve("logged in");
      } else {
        reject("invalid something");
      }
    } catch {
      reject("Please, try again later");
    }
  });
};

it gives me an error that this.cleanup() is not a function, and after a few tries, I realized that async somehow change "this".

can you please help me, where did I made an error?

Alex Rudenko
  • 33
  • 7
  • 2
    why you have promises and async/await in one place? – demkovych Jun 25 '20 at 14:24
  • 1
    _I realized that async somehow change "this"._ async has nothing to do with how value of `this` is set – Yousaf Jun 25 '20 at 14:26
  • 1
    This has nothing to do with use of the `async` keyword. If `this` in this method is supposed to be a `User` object, then there are a couple places you might be going wrong. For starters, `User.prototype.login` should NOT be an arrow function because that won't pass the instance value in `this`. It should be a regular function declaration And 2nd, you have to make sure you're calling `someUserObj.login()` appropriately so you'd have to show us that calling code for us to see if you're doing that correctly. – jfriend00 Jun 25 '20 at 14:32
  • Please avoid the [explicit Promise constructor antipattern](https://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it) – VLAZ Jun 25 '20 at 16:54

1 Answers1

3

2 problems in your code:

  1. Functions defined on a prototype object should be regular functions instead of arrow functions because functions defined on objects are used as methods, i.e. value of this inside them refers to instances of the constructor function or class. Using an arrow function will set incorrect value of this and that is most likely the cause of error in your code.

  2. You have mixed promise-chaining with async-await syntax.async functions always return a Promise, so instead of explicitly returning a Promise object, make login function an async function and return the string from the function which is same as calling resolve() function with the string. To reject a promise, throw an error from an async function.

This is how you should define login function

User.prototype.login = async function() {
    this.cleanup();

    try {
      const attemptedUser = await usersCollection.findOne({username: this.data.username});

      if (attemptedUser && attemptedUser.password == this.data.password) {
        return "logged in";
      }

      throw new Error("invalid something");

    } catch (error) {
      console.log("Please, try again later");
      throw error;
    }
};
Yousaf
  • 27,861
  • 6
  • 44
  • 69
  • This is generally true (though you should be throwing error objects for rejections), but how does this fix the value of `this` in `this.cleanup()` which is the main point of the question? – jfriend00 Jun 25 '20 at 14:33
  • Replacing a promise rejection with `return` or `console.log` isn't quite desirable. You should throw an error, in an async function this makes the promise reject with the error object, which is much closer to the original behavior and is a good pattern. – Klaycon Jun 25 '20 at 14:34
  • @jfriend00 it has nothing to do with the value of `this` as i have mentioned in my answer that value of `this` inside `login` function will be same as the surrounding environment in which this function is defined. – Yousaf Jun 25 '20 at 14:35
  • @Klaycon i agree. Edited, was missing a `throw` statement. – Yousaf Jun 25 '20 at 14:38
  • Yeah, and that lexical value of `this` from the arrow function is likely wrong. I don't believe that their code breaks when all they change is the `async` keyword. – jfriend00 Jun 25 '20 at 14:39
  • @Yousaf I was thinking `throw new Error("invalid something")` and `throw new Error("Please try again later")` as these would match the original behavior of rejecting with those messages. Throwing the error that was caught is better, but I still don't like the promise resolving with "invalid something" instead of rejecting – Klaycon Jun 25 '20 at 14:39
  • @jfriend00 agreed. Most likely cause of error here is the use of arrow function as a method on `User` object. – Yousaf Jun 25 '20 at 15:06
  • Just want to add that you shouldn't have the keyword ```function``` on an arrow function. Just have it as an actual function to preserve the semantics of ```this``` – Benz Stevox Jun 25 '20 at 15:21
  • @BenzStevox actually that arrow was a typo. Thanks for pointing it out. – Yousaf Jun 25 '20 at 15:23
  • @Yousaf thank you! it worked, but partially :) now I got 2 concerns: 1) why did first code with promises and .then worked fine, despite having arrow function 2) how can I access these errors later on? I call this from userController: `exports.login = async function (req, res) { let user = new User(req.body); try { res.send(await user.login()); } catch (error) { res.send(error); } };` these `"invalid something"` and `"Please, try again later"` are supposed to return on front end I'm just starting to learn, and code is oversimplified, obviously – Alex Rudenko Jun 25 '20 at 20:02
  • Without some background knowledge of where `cleanup` function is defined, i can't tell you why first version of your code worked. However, you should **never use arrow functions where you expect the function to be used as a method**. Since you are calling the function from an async function, if the `user.login` function throws an error, promise will be rejected and the `catch` block of your controller will be invoked with the error object. To access the error message, use `message` property of the error object. – Yousaf Jun 26 '20 at 11:57