Is it bad practice to have a constructor function return a Promise?

Question

I'm trying to create a constructor for a blogging platform and it has many async operations going on inside. These range from grabbing the posts from directories, parsing them, sending them through template engines, etc.

So my question is, would it be unwise to have my constructor function return a promise instead of an object of the function they called new against.

For instance:

var engine = new Engine({path: '/path/to/posts'}).then(function (eng) {
   // allow user to interact with the newly created engine object inside 'then'
   engine.showPostsOnOnePage();
});

Now, the user may also not supply a supplement Promise chain link:

var engine = new Engine({path: '/path/to/posts'});

// ERROR
// engine will not be available as an Engine object here

This could pose a problem as the user may be confused why engine is not available after construction.

The reason to use a Promise in the constructor makes sense. I want the entire blog to be functioning after the construction phase. However, it seems like a smell almost to not have access to the object immediately after calling new.

I have debated using something along the lines of engine.start().then() or engine.init() which would return the Promise instead. But those also seem smelly.

Edit: This is in a Node.js project.

Neutralization

Is creating the object the asynchronous operation or is acquiring its resources really the asynchronous operation? I think you wouldn't have this problem if you used DI – Homeopathy

The most common design pattern I've seen for this type of issue is to just create your object shell in the constructor and then do all the async stuff in an .init() method that can then return the promise. Then you separate out instance data in the object and construction of that object from the async initialization operation. The same issue arises when you have all sorts of different errors (which the caller wants to handle differently) that can occur in the initialization of the object. Much better to return the object from the constructor and then use .init() to return other things. – Remembrancer

I'm totally agree with jfriend00. It's better practice to use an init method to make a promise! – Chaddie

@Remembrancer I still don't see why. More code to write and maintain in this approach. – Floweret

@KarlMorrison - For a discussion of the various techniques to do something asynchronous when creating a new object, see Asynchronous Operations in Constructor. My personal recommendation is a factory function that returns a promise because there is no way to accidentally misuse that pattern and the interface is clear and obvious. – Remembrancer

For me, this is not different from misusing any other method. What if a method returns a promise and the caller don't provide .then or use async?. Methods (including constructors) imply a contract. As long as your constructor is well document, warning that it returns a promise and showing an example on how it is expected to be used, it is no longer your responsibility. – Pavo

Answer 1

Yes, it is a bad practise. A constructor should return an instance of its class, nothing else. It would mess up the new operator and inheritance otherwise.

Moreover, a constructor should only create and initialize a new instance. It should set up data structures and all instance-specific properties, but not execute any tasks. It should be a pure function without side effects if possible, with all the benefits that has.

What if I want to execute things from my constructor?

That should go in a method of your class. You want to mutate global state? Then call that procedure explicitly, not as a side effect of generating an object. This call can go right after the instantiation:

var engine = new Engine()
engine.displayPosts();

If that task is asynchronous, you can now easily return a promise for its results from the method, to easily wait until it is finished.
I would however not recommend this pattern when the method (asynchronously) mutates the instance and other methods depend on that, as that would lead to them being required to wait (become async even if they're actually synchronous) and you'd quickly have some internal queue management going on. Do not code instances to exist but be actually unusable.

What if I want to load data into my instance asynchronously?

Ask yourself: Do you actually need the instance without the data? Could you use it somehow?

If the answer to that is No, then you should not create it before you have the data. Make the data ifself a parameter to your constructor, instead of telling the constructor how to fetch the data (or passing a promise for the data).

Then, use a static method to load the data, from which you return a promise. Then chain a call that wraps the data in a new instance on that:

Engine.load({path: '/path/to/posts'}).then(function(posts) {
    new Engine(posts).displayPosts();
});

This allows much greater flexibility in the ways to acquire the data, and simplifies the constructor a lot. Similarly, you might write static factory functions that return promises for Engine instances:

Engine.fromPosts = function(options) {
    return ajax(options.path).then(Engine.parsePosts).then(function(posts) {
        return new Engine(posts, options);
    });
};

…

Engine.fromPosts({path: '/path/to/posts'}).then(function(engine) {
    engine.registerWith(framework).then(function(framePage) {
        engine.showPostsOn(framePage);
    });
});

Biel

How exactly does it "mess up the new operator and inheritance", please? Of course, it returns a promise that resolves to the instance instead of an instance. Could you please explain what this has to do with inheritance? – Gallon

@mightyiam Well, when (new Engine) instanceof Engine is false, that's definitely unexpected. Similarly, when you try to inherit from that class, super(…) initialises this with a promise not an engine instance - a mess. Don't return from constructors. – Biel

@Biel this is great! but what if the path to load the data is declared in the instance constructor, in your example {path: '/path/to/posts'} this is statically declared, what if you you declare the path within the constructor so the only way to get it is to instantiate it. – Hixson

@JordanDavis Why can't you move the declaration of the path into the static load function? You might want to ask a new question to share your code and be specific about the goal you have and restrictions you face. – Biel

@Biel thanks for the response, yeah I started to get what you were saying in your question about just creating a static load method, also could use the new await if you declare a static async so like let data = await Engine.load then just pass into constructor. – Hixson

@Bergi, a great solution, but can I adapt it to work with a singleton? I mean, a singleton that calls the async factory method and returns a promise. Something like this: #59612576 – Leggy

Answer 2

I encountered the same problem and came up with this simple solution.

Instead of returning a Promise from the constructor, put it in this._initialized property, like this:

function Engine(path) {
  this._initialized = Promise.resolve()
    .then(() => {
      return doSomethingAsync(path)
    })
    .then((result) => {
      this.resultOfAsyncOp = result
    })
}
  

Then, wrap every method in a callback that runs after the initialization, like that:

Engine.prototype.showPostsOnPage = function () {
  return this._initialized.then(() => {
    // actual body of the method
  })
}

How it looks from the API consumer perspective:

engine = new Engine({path: '/path/to/posts'})
engine.showPostsOnPage()

This works because you can register multiple callbacks to a promise and they run either after it resolves or, if it's already resolved, at the time of attaching the callback.

This is how mongoskin works, except it doesn't actually use promises.

---

Edit: Since I wrote that reply I've fallen in love with ES6/7 syntax so there's another example using that.

class Engine {
  
  constructor(path) {
    this._initialized = this._initialize(path)
  }

  async _initialize() {
    // actual async constructor logic
    this.resultOfAsyncOp = await doSomethingAsync(path)
  }

  async showPostsOnPage() {
    await this._initialized
    // actual body of the method
  }
  
}

Absently

Hm, I don't like this pattern because of the need to "wrap every method". This is just unecessary overhead most times, and complicates many things when methods do return promises while they usually wouldn't need to. – Biel

I created npm module which makes wrapping automatic: npmjs.com/package/synchronisify – Powered

I know this is an old thread, but to avoid the "wrap every method" issue, at least in Node, a Proxy was useful. – Ellaelladine

Can also be done with a Proxy instead of having to call await this._initialized in every method – Rupiah

@Rupiah Nice! How would you do that? – Gooch

@Gooch something like this gist.github.com/purefan/9a0d29c6123341baa37024c369a96562 doesnt work super nice with promises (the get trap must return a function if you're calling a function but async methods return a promise) – Rupiah

Answer 3

To avoid the separation of concerns, use a factory to create the object.

class Engine {
    constructor(data) {
        this.data = data;
    }

    static makeEngine(pathToData) {
        return new Promise((resolve, reject) => {
            getData(pathToData).then(data => {
              resolve(new Engine(data))
            }).catch(reject);
        });
    }
}

Duwe

Avoid the Promise constructor antipattern! – Biel

Answer 4

The return value from the constructor replaces the object that the new operator just produced, so returning a promise is not a good idea. Previously, an explicit return value from the constructor was used for the singleton pattern.

The better way in ECMAScript 2017 is to use a static methods: you have one process, which is the numerality of static.

Which method to be run on the new object after the constructor may be known only to the class itself. To encapsulate this inside the class, you can use process.nextTick or Promise.resolve, postponing further execution allowing for listeners to be added and other things in Process.launch, the invoker of the constructor.

Since almost all code executes inside of a Promise, errors will end up in Process.fatal

This basic idea can be modified to fit specific encapsulation needs.

class MyClass {
  constructor(o) {
    if (o == null) o = false
    if (o.run) Promise.resolve()
      .then(() => this.method())
      .then(o.exit).catch(o.reject)
  }

  async method() {}
}

class Process {
  static launch(construct) {
    return new Promise(r => r(
      new construct({run: true, exit: Process.exit, reject: Process.fatal})
    )).catch(Process.fatal)
  }

  static exit() {
    process.exit()
  }

  static fatal(e) {
    console.error(e.message)
    process.exit(1)
  }
}

Process.launch(MyClass)

Reorganize

Answer 5

This is in typescript, but should be easily converted to ECMAscript

export class Cache {
    private aPromise: Promise<X>;
    private bPromise: Promise<Y>;
    constructor() {
        this.aPromise = new Promise(...);
        this.bPromise = new Promise(...);
    }
    public async saveFile: Promise<DirectoryEntry> {
        const aObject = await this.aPromise;
        // ...
        
    }
}

The general pattern is to store the promises as internal variables using the constructor and await for the promises in the methods and make the methods all return promises. This allows you to use async/await to avoid long promise chains.

The example I gave is good enough for short promises, but putting in something that requires a long promise chain will make this messy, so to avoid that create a private async method that will be called by the constructor.

export class Cache {
    private aPromise: Promise<X>;
    private bPromise: Promise<Y>;
    constructor() {
        this.aPromise = initAsync();
        this.bPromise = new Promise(...);
    }
    public async saveFile: Promise<DirectoryEntry> {
        const aObject = await this.aPromise;
        // ...
        
    }
    private async initAsync() : Promise<X> {
        // ...
    }

}

Here is a more fleshed out example for Ionic/Angular

import { Injectable } from "@angular/core";
import { DirectoryEntry, File } from "@ionic-native/file/ngx";

@Injectable({
    providedIn: "root"
})
export class Cache {
    private imageCacheDirectoryPromise: Promise<DirectoryEntry>;
    private pdfCacheDirectoryPromise: Promise<DirectoryEntry>;

    constructor(
        private file: File
    ) {
        this.imageCacheDirectoryPromise = this.initDirectoryEntry("image-cache");
        this.pdfCacheDirectoryPromise = this.initDirectoryEntry("pdf-cache");
    }

    private async initDirectoryEntry(cacheDirectoryName: string): Promise<DirectoryEntry> {
        const cacheDirectoryEntry = await this.resolveLocalFileSystemDirectory(this.file.cacheDirectory);
        return this.file.getDirectory(cacheDirectoryEntry as DirectoryEntry, cacheDirectoryName, { create: true })
    }

    private async resolveLocalFileSystemDirectory(path: string): Promise<DirectoryEntry> {
        const entry = await this.file.resolveLocalFilesystemUrl(path);
        if (!entry.isDirectory) {
            throw new Error(`${path} is not a directory`)
        } else {
            return entry as DirectoryEntry;
        }
    }

    public async imageCacheDirectory() {
        return this.imageCacheDirectoryPromise;
    }

    public async pdfCacheDirectory() {
        return this.pdfCacheDirectoryPromise;
    }

}

Conservatoire