Node.js: Returning original Promise vs. a new Promise instance

Question

I am trying to fix some code that is really Promise heavy I am trying to fix is by removing the new Promise declaration and returning the initial Promise instead, but am trying to make sure that I am properly tanslating the before and after. Below I have an example outlining one of the scenarios that need to be changed. Am I missing something between the before and after code or am I traveling in the right direction? Also,for the after, will I be able to catch all of the rejected errors?

Before:

    add(book: IBook, authorId: string): Promise<IBookModel> {
        let p = new Promise((reject, resolve) => {
            let newBook: IBook = new Book({…book});
            newBook.createInitialSetup();
            let bookReady = {... newBook};
            this.bookRepository.add(bookReady)
                .then((completedBook: IBookModel) => {
                    this.bookRepository.findAuthorById(authorId)
                        .then((author: IAuthorModel) => {
                            let newBookAuthor: IAuthorModel = new Author();
                            newBookAuthor(completedBook._id, author._id);

                            let finalBookAuthor = {... newBookAuthor} as IAuthor;

                            this.bookRepository.addBookAuthor(finalBookAuthor)
                                .then((result: IBookAuthorModel) => {
                                    resolve(completedBook);
                                })
                                .catch((err: Error) => {
                                    reject(err);
                                });
                        })
                        .catch((err: Error) => {
                            reject(err);
                        });
                })
                .catch((err: Error) => {
                    reject(err);
                });
        });

        return p;
    }

After:

    add(book: IBook, authorId: string): Promise<IBookModel> {
         let groupSetup: IGroupModel = new Group({...group});
         newBook.createInitialSetup();
         let bookReady = {... newBook};

         return this.bookRepository.add(bookReady)
                    .then((completedBook: IBookModel) => {
                        return this.bookRepository.findAuthorById(authorId)
                                   .then((author: IAuthorModel) => {
                                        let newBookAuthor: IAuthorModel = new Author();
                                         newBookAuthor(completedBook._id, author._id);

                                        let finalBookAuthor = {... newBookAuthor} as IAuthor;

                                        return this.bookRepository.addBookAuthor(finalBookAuthor)
                                                    .then((result: IBookAuthorModel) => {
                                                           return completedBook;
                                         });                  
                        });
         });
    }

Just as an aside, that code looks like the poster child example for `async/await` — Titian Cernicova-Dragomir, Jan 29 '18 at 18:12
that code needs a lot of refactoring. You arent making use of the promise chain at all. You dont need a `new Promise` for example, just return the one of `this.bookRepository.add` — Johannes Merz, Jan 29 '18 at 18:23
@Johannes Merz That what I am trying to do in the after code. Is that code more indicative of what you are referring to? Because it seems like I have to account for the inner promises. — user1790300, Jan 29 '18 at 18:26
@Titian Cernicova-Dragomir I have not dealt with await/async yet, I would need an example of how the code above would translate to an await/async example. — user1790300, Jan 29 '18 at 18:29
@user1790300, ok I'll convert it, and provide it as an answer — Titian Cernicova-Dragomir, Jan 29 '18 at 18:31
Yes, you're doing a fine job at [avoiding the `Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! Only you should fix your indentation now. — Bergi, Jan 29 '18 at 18:44
@Bergi Honestly, I realized that I created the antipattern is my code and am trying to fix it the right way.:-) — user1790300, Jan 29 '18 at 18:47

score 3 · Answer 1 · answered Jan 29 '18 at 18:28

3

Am I missing something between the before and after code or am I traveling in the right direction?

Traveling in the right direction.

More

Also don't nest then calls. Chain then:

add(book: IBook, authorId: string): Promise<IBookModel> {
     let groupSetup: IGroupModel = new Group({...group});
     newBook.createInitialSetup();
     let bookReady = {... newBook};

     return this.bookRepository
                .add(bookReady)
                .then((completedBook: IBookModel) => {
                    return this.bookRepository.findAuthorById(memberId)
                })
                .then(///so on

answered Jan 29 '18 at 18:28

basarat

261,912
58
460
511

1

In this example, how would I account for the final thing that needs to return needs to be the bookReady object, because that represents the newly created book, because it looks like the author would be returned to the next then. Am I correct in this assumption? Also, if a rejection occurs in the first then case, would that automatically bubble to outside of this method? – user1790300 Jan 29 '18 at 18:42
The final value will bubble. `Also, if a rejection occurs in the first then case, would that automatically bubble to outside of this method` yes. – basarat Jan 29 '18 at 18:43
Gotcha. I am also assuming that the second then would handle adding the bookAuthor.add. How would I pass the completedBook to the second then so that I could return that object when the final save occurs? – user1790300 Jan 29 '18 at 18:53

Titian Cernicova-Dragomir · Accepted Answer · 2018-01-29T18:53:00.560

Async/await might make your code easier to understand and read:

async add(book: IBook, authorId: string): Promise<IBookModel> {
    let newBook: IBook = new Book({ ...book });
    newBook.createInitialSetup();
    let bookReady = { ...newBook };
    let completedBook: IBookModel = await this.bookRepository.add(bookReady)
    let author: IAuthorModel = await this.bookRepository.findAuthorById(memberId)

    let newBookAuthor: IAuthorModel = new Author();
    newBookAuthor(completedBook._id, author._id);

    let finalBookAuthor = { ...newBookAuthor } as IAuthor;

    let result: IBookAuthorModel = await this.bookRepository.addBookAuthor(finalBookAuthor);
    return completedBook
}

Just a couple of notes:

Not sure who memberId is it is not defined in the original code
You could surround with try/catch the whole code but since all you do is forward the error, this is not needed as the returned Promise will be rejected if any of the awaited promises will fail
Take it with a grain of salt, the general principle of using async/await is valid, but the conversion was performed under a screaming baby background so it might have errors.

Thanks, I definitely understand the crying baby in the background. So in this scenario, if any rejections occur in any of the repository calls, will the code stop at that point and return the catch to the caller of the add method? — user1790300, Jan 29 '18 at 19:00
Yes it will be surfaced as a rejection of the returned `Promise` — Titian Cernicova-Dragomir, Jan 29 '18 at 19:02

Node.js: Returning original Promise vs. a new Promise instance

2 Answers2

More