13

getUser is an async function? if it is going to take longer time to resolve? is it going to always return the right value in my someotherclass.

class IdpServer {
    constructor() {
        this._settings = {
            // some identity server settings.
        };
        this.userManager = new UserManager(this._settings);
        this.getUser();
    }

    async getUser() {
        this.user = await this.userManager.getUser();
    }

    isLoggedIn() {
        return this.user != null && !this.user.expired;
    }
}

let idpServer = new IdpServer();
export default idpServer;


// another class 
// import IdpServer from '...'
 class SomeOtherClass {
     constructor() {
        console.log(IdpServer.isLoggedIn());
     }
 }
hashbytes
  • 769
  • 1
  • 8
  • 26
  • 5
    Constructors should generally be used to get an object into a usable, valid state. Not to do lots of work. – James Thorpe Apr 06 '18 at 14:12
  • 1
    @JamesThorpe Adding on that, a constructor builds an object, it doesn't promise to build part of an object – Alexandre Beaudet Apr 06 '18 at 14:13
  • I think you're going to have to make the client code `await` the call to `getUser()` before checking the status. – Pointy Apr 06 '18 at 14:13
  • What does `getUser` do actually? – Jonas Wilms Apr 06 '18 at 14:15
  • it get user object from local storage, and at this point of time, getUser is only being called in constructor. – hashbytes Apr 06 '18 at 14:16
  • @JamesThorpe Alexandre Any better approaches to make sure, it always returns right result? – hashbytes Apr 06 '18 at 14:17
  • 2
    Duplicates : https://stackoverflow.com/questions/39028882/chaining-async-method-calls-javascript, https://stackoverflow.com/questions/37351146/asynchronous-data-loading-in-class-constructor/, https://stackoverflow.com/questions/24398699/is-it-bad-practice-to-have-a-constructor-function-return-a-promise – Felix Kling Apr 06 '18 at 14:19
  • @hashbytes localStorage (if that's what you mean) is synchronous. – Estus Flask Apr 06 '18 at 14:41
  • [Just don't](https://stackoverflow.com/q/24398699/1048572). – Bergi Apr 06 '18 at 14:42

1 Answers1

17

This is a problem that is related to this popular question.

Once a code is asynchronous, it cannot be used in synchronous manner. If the use of raw promises is unwanted, all control flow should be performed with async functions.

The problem here is that getUser provides a promise of user data, not user data itself. A promise is lost in constructor, and this is antipattern.

One way to solve the problem is to provide initialization promise for IdpServer, while the rest of API will be synchronous:

class IdpServer {
    constructor() {
        ...
        this.initializationPromise = this.getUser(); 
    }

    async getUser() {
        this.user = await this.userManager.getUser();
    }

    isLoggedIn() {
        return this.user != null && !this.user.expired;
    }
}

// inside async function
await idpServer.initializationPromise;
idpServer.isLoggedIn();

Depending on how the application works, IdpServer.initializationPromise can be handled on application initialization to guarantee that all units that depend on IdpServer won't be initialized until it's ready.

Another way is to make IdpServer entirely asynchronous:

class IdpServer {
    constructor() {
        ...
        this.user = this.getUser(); // a promise of user data
    }

    async getUser() {
        return this.userManager.getUser();
    }

    async isLoggedIn() {
        const user = await this.user;
        return user != null && !user.expired;
    }
}

// inside async function
await idpServer.isLoggedIn();

It's expected that all units that depend on it will also have asynchronous API.

Estus Flask
  • 206,104
  • 70
  • 425
  • 565
  • This is new implementation, @estus this is being int a function, which was being used in 20 places at least in the application. Just thinking if there is an easier way so that it require minimal changes. – hashbytes Apr 06 '18 at 16:06
  • Since this was design mistake, this should be refactored. If a promise exists, there should be a way to chain it in places that depend on the results of this promise. I guess the first way requires less changes and is generally more common. – Estus Flask Apr 06 '18 at 16:23
  • 1
    Just want to let you guys know, I fixed it by making this as the first thing to run and only when this resolves, then router will render the page template. – hashbytes Apr 18 '18 at 17:17
  • @hashbytes - how can you test for when the promise resolves? – Jason Cheng Feb 14 '23 at 21:57