-1

I am trying to make a new class like this:

const request = require("request");

class Person {
    constructor(personName) {
        request(`http://personapi.com/name/${personName}`, (err, res, body) => {
            body = JSON.parse(body);
            this.name = body.name;
            this.age = body.age;
            this.gender = body.gender;
        }
    }
}

let person = new Person("Donald Trump");
console.log(person.name);

Doing the above does not work, because it creates the new person which has no attributes yet, because the request is not done loading yet. If I do something like:

let person = new Person("Donald Trump");
setTimeout(() => {
    console.log(person.name);
}, 1000);

It works fine. I know this is because it's asynchronous. How do I make sure let person is not actually set, before the request is done? Don't worry about code blocking.

MortenMoulder
  • 6,138
  • 11
  • 60
  • 116
  • 6
    [Don't put asynchronous code in your constructor](https://stackoverflow.com/q/24398699/1048572). – Bergi Oct 16 '17 at 21:40
  • *"How do I make sure let person is not actually set, before the request is done?"* Not with this code. An assignment operation cannot be interrupted. – Felix Kling Oct 16 '17 at 21:41
  • @Bergi Very valid yes. In this case, I will only be creating a new person and sending the request as above. – MortenMoulder Oct 16 '17 at 21:46
  • @FelixKling I often see libraries with code style like the bottom two lines. How are they achieving that? – MortenMoulder Oct 16 '17 at 21:47
  • @MortenMoulder: Hard to say without seeing the actual code. Have a look at [my answer here](https://stackoverflow.com/a/37351311/218196). I recommend to create a static method on the class. – Felix Kling Oct 16 '17 at 21:48

2 Answers2

3

Maybe just do a request before instantiating a Person:

const request = require("request");

class Person {
    constructor(obj) {
            this.name = obj.name;
            this.age = obj.age;
            this.gender = obj.gender;
    }
}

let person;

request(`http://personapi.com/name/${personName}`, (err, res, body) => {
    body = JSON.parse(body);
    person = new Person(body);
});
dhilt
  • 18,707
  • 8
  • 70
  • 85
  • Sorry, that is not possible. I want everything to be inside the class. – MortenMoulder Oct 16 '17 at 21:44
  • @MortenMoulder Then put the method that does the request in the class. Just don't put it on an instance - it needs to happen before the instantiation. – Bergi Oct 16 '17 at 21:48
1

I'd suggest storing the promise in property:

const request = require("request");

class Person {
    constructor(personName) {
        this.ready = 
          request(`http://personapi.com/name/${personName}`, (err, res, body) => {
              body = JSON.parse(body);
              this.name = body.name;
              this.age = body.age;
              this.gender = body.gender;
          });
    }
}

let person = new Person("Donald Trump");
person.ready.then(_ => console.log(person.name));

Although seems a little hacky. Another option that looks better and also encapsulates person related logic inside Person class:

const request = require("request");

class Person {
    constructor({name, age, gender}) {
      this.name   = name;
      this.age    = age;
      this.gender = gender;
    }

    static fromName(name) {
      return request(`http://personapi.com/name/${personName}`)
        .then(resp => resp.json())
        .then(body => new this(body));
    }
}

Person
  .fromName("Donald Trump")
  .then(person => console.log(person.name);
ichigolas
  • 7,595
  • 27
  • 50
  • 1
    That's kind of an ugly approach, if I have to be honest. – MortenMoulder Oct 16 '17 at 21:44
  • 1
    Constructor should not return a promise. Or store it or be async in anyway. – AtheistP3ace Oct 16 '17 at 21:44
  • I don't think `request` returns a promise - you can see it taking a callback – Bergi Oct 16 '17 at 21:46
  • If you store the promise, then the data should stay inside the promise. Assigning to `this.name`, `.age` and `.gender` is still wrong. – Bergi Oct 16 '17 at 21:47
  • `new this(body)` from a `static` method?! -- reeeeally confusing – Igor Soloydenko Oct 16 '17 at 21:49
  • @AtheistP3ace `this.ready = request(...)` is returning a promise? – Igor Soloydenko Oct 16 '17 at 21:50
  • @another-guy But that's how it works with inheritance. You'd better get accustomed to the pattern :-) – Bergi Oct 16 '17 at 21:50
  • @Bergi Could you point me to the pattern, I'm new to ES features. Aslo, I hate inheritance (in any language) :) – Igor Soloydenko Oct 16 '17 at 21:51
  • @another-guy at first glance i thought they were returning a promise. I realized I had seen it incorrectly and edited my comment. To the point. dont return, store or put any async related code in the constructor. That is just terrible in my opinion. – AtheistP3ace Oct 16 '17 at 21:52
  • @AtheistP3ace I don't see how `this.ready = request(...)` is bad. I read it as a way to compose a new function and put it into the `ready` field. The asynchronous operation will be invoked later on when the consumer does `.then(...)`. Not sure why it is confusing. Now. I'm new to JS so I understand that this approach may be not very idiomatic. – Igor Soloydenko Oct 16 '17 at 21:56
  • This asyncronous initialization of instances is pretty common in API consumer libraries. This library does that: https://marmelab.com/blog/2015/03/10/deal-easily-with-your-rest-api-using-restful-js.html. I don't think there are prettier alternatives. – ichigolas Oct 16 '17 at 21:59
  • @another-guy a constructor should initialize a new instance not be making requests for data. Add an init method to create a promise. There are many other design patterns to get this data and populate the object with it but you shouldnt be making async calls in the constructor to do it. But that is just my opinion and the way I learned things. To each his own I suppose but seems wrong to me. – AtheistP3ace Oct 16 '17 at 22:00
  • @AtheistP3ace does it _really_ requests for the data though? I thought unless `.then()` is invoked, the request will NEVER happen. Am I wrong? – Igor Soloydenko Oct 16 '17 at 22:01
  • @another-guy i could be wrong but I think the request is made immediately. the request is not dependent on the promise. You can use the promise or not. The request still happens. – AtheistP3ace Oct 16 '17 at 22:03