Array undefined after resolving it after for loop

Question

I have the problem of having an undefined array which gets resolved after a for loop where it gets filled. It looks like the following:

function mainFunction() {
    getUnreadMails().then(function(mailArray) {
        // Do stuff with the mailArray
        // Here it is undefined
    })
}

function getUnreadMails() {
    var mailArray = [];
    return new Promise(function(resolve, reject) {

        listMessages(oauth2Client).then(
            (messageIDs) => {

                for(var i = 0; i < messageIDs.length; i++) {
                    getMessage(oauth2Client, 'me', messageIDs[i]).then(function(r) {
                        // Array gets filled
                        mailArray.push(r);
                    }, function(error) {
                        reject(error);
                    })
                }
                // Array gets resolved
                resolve(mailArray);
            },
            (error) => {
                reject(error);
            }
        )
    });
}

Both listMessages() and getMessage() returns a promise, so it is chained here. Any ideas why I am getting an undefined mailArray? My guess is that it is not filled yet when it gets resolved. Secondly I think this flow is not a good practice.

`getMessage` looks like it is asynchron. this wont work, because you're calling resolve before the callback of `getMessage` is called — Joshua K, Sep 27 '17 at 13:42
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/all — epascarello, Sep 27 '17 at 13:42
@JoshuaK Exactly what I think too. How can this be done correct? — ffritz, Sep 27 '17 at 13:42
Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! And always use `Promise.all` on loops — Bergi, Sep 27 '17 at 13:46
resolve it when all messages are collected instead of after the loop. You can check if (messageIDs.length equals mailArray.length). If this is true all messages are collected and you can resolve. EDIT: The `Promise.all(...).then(resolve)` solution is the better way if you understand it. — Joshua K, Sep 27 '17 at 13:50

Thomas · Accepted Answer · 2017-09-27T14:38:10.367

The Array is probably undefined because it is never defined; at least nowhere in your code. And your promise resolves before any iteration in your loop can resolve or better said, throw (trying to push to undefined).

Besides that. you can highly simplyfy your code by using Array#map and Promise.all.

And there's no point in catching an Error just to rethrow the very same error without doing anything else/with that error.

function getUnreadMails() {
    //put this on a seperate line for readability
    //converts a single messageId into a Promise of the result
    const id2message = id => getMessage(oauth2Client, 'me', id);

    return listMessages(oauth2Client)
        //converts the resolved messageId into an Array of Promises
        .then(messageIDs => messageIDs.map( id2message ))
        //converts the Array of Promises into an Promise of an Array
        //.then(Promise.all.bind(Promise));
        .then(promises => Promise.all(promises));
    //returns a Promise that resolves to that Array of values
}

or short:

function getUnreadMails() {
    return listMessages(oauth2Client)
        .then(messageIDs => Promise.all(messageIDs.map( id => getMessage(oauth2Client, 'me', id) )))
}

.then(Promise.all) won't work

I wanted to make the intermediate results more clear by seperating them into distinct steps/functions. But I typed too fast and didn't check it. Fixed the code.

In the short version, where does the mailArray then actually get filled/resolved

Promise.all() takes an an Array of promises and returns a single promise of the resolved values (or of the first rejection).

messageIDs.map(...) returns an Array and the surrounding Promise.all() "converts" that into a single Promise of the resolved values. And since we return this Promise inside the Promise chain, the returned promise (listMessages(oauth2Client).then(...)) also resolves to this Array of values.

I really like this approach, thanks! In the short version, where does the mailArray then actually get filled/resolved? — ffritz, Sep 27 '17 at 13:54
`.then(Promise.all)` won't work, it would need to be bound to `Promise`. Better move it inside the arrow function. — Bergi, Sep 27 '17 at 13:57
Okay this definitely works, thanks a lot! Still trying to understand promise chaining fully, but this helps getting rid of a lot of the mentioned anti pattern code. — ffritz, Sep 27 '17 at 14:29
@ffritz updated the answer to address the question. Hope this makes it clearer. Also fixed the error with `.then(Promise.all)` that Bergi mentioned. — Thomas, Sep 27 '17 at 14:39

marvel308 · Answer 2 · 2017-09-27T13:48:30.530

            getMessage(oauth2Client, 'me', messageIDs[i]).then(function(r) {
                    // Array gets filled
                    mailArray.push(r);
                }, function(error) {
                    reject(error);
                })

is an asynchronous call

resolve(mailArray);

won't wait for it to push data and would resolve the array before hand

to resole this you should use Promise.all()

function mainFunction() {
    getUnreadMails().then(function(mailArray) {
        // Do stuff with the mailArray
        // Here it is undefined
    })
}

function getUnreadMails() {
    var mailArray = [];

    return listMessages(oauth2Client).then(
        (messageIDs) => {

            for(var i = 0; i < messageIDs.length; i++) {
                mailArray.push(getMessage(oauth2Client, 'me', messageIDs[i]));
            }
            // Array gets resolved
            return Promise.all(mailArray);
        },
        (error) => {
            reject(error);
        }
        )
}

James Long · Answer 3 · 2017-09-27T13:47:27.470

Just picking up on marvel308's answer, I think you need to create a new Promise that resolves when your other ones do. I haven't had a chance to test this, but I think this should work

function getUnreadMails() {

    var mailArray = [];

    return new Promise(function(resolve, reject) {

        listMessages(oauth2Client).then(
            (messageIDs) => {

                var messages = [];

                for(var i = 0; i < messageIDs.length; i++) {
                    messages.push(
                        getMessage(oauth2Client, 'me', messageIDs[i]).catch(reject)
                    );
                }

                Promise.all(messages).then(resolve);

            },
            (error) => {
                reject(error);
            }
        )
    });

}

This way, the resolve of your first promise gets called when all the messages have resolved

You still will need to avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! — Bergi, Sep 27 '17 at 13:47
Good shout. I was looking for a quick solution but you're right - we should avoid anti-patterns — James Long, Sep 27 '17 at 13:52

kharandziuk · Answer 4 · 2017-09-27T14:23:01.740

0

Explicit construction is an anti-pattern

I believe you can write that piece of code much shorter and, IMHO, cleaner

function mainFunction() {
  getUnreadMails().then(function(mailArray) {
    // Do stuff with the mailArray
    // Here it is undefined
  })
}

function getUnreadMails() {
  return listMessages(oauth2Client)
      .then((messageIDs) => Promise.all(messageIDs.map(id => getMessage(oauth2Client, 'me', id)))

}

edited Sep 27 '17 at 14:23

answered Sep 27 '17 at 13:51

kharandziuk

12,020
17
63
121

1

`.then(Promise.all)` won't work, it would need to be bound to `Promise`. Better move it inside the arrow function. – Bergi Sep 27 '17 at 13:57

Sergio Baidon · Answer 5 · 2017-09-27T14:13:57.723

0

Since your getMessage function is async as well you need to wait until all your calls finish.

I would suggest using Promise.all

Here you can find more info: MDN Promise.all()

The code would look something like this:

messageIDs.map(...) returns an array of Promises

use Promise.all() to get an array with all the promises responses

resolve if values are correct reject otherwise

function mainFunction() {
      getUnreadMails().then(function(mailArray) {
        // Do stuff with the mailArray
        // Here it is undefined
      })
  }

  function getUnreadMails() {
    return new Promise(function(resolve, reject) {
        listMessages(oauth2Client).then(
            (messageIDs) => {
                return Promise.all(messageIDs.map(id => getMessage(oauth2Client, 'me', id)))
       })
      .then((messages) => resolve(messages))
      .catch(error => reject(error))
    });
}

One thing to keep in mind is that Promise.all() rejects if any of your promises failed

Hope this helps!

edited Sep 27 '17 at 14:13

answered Sep 27 '17 at 14:02

Sergio Baidon

113
7

I see, this makes sense now, thanks! The resolve part is missing in Thomas answer in the short part below I think. – ffritz Sep 27 '17 at 14:04
Actually I think his answer is great, I'm pretty should it would work as well – Sergio Baidon Sep 27 '17 at 14:06
I think people are thinking my getMessage() returns an array of messages, but it actually returns one message, which needs to be stored in the mailArray, and then after every message is retrieved the mailArray needs to be resolved. – ffritz Sep 27 '17 at 14:09
I think we did get that part, we are assuming your `getMessage()` will return a Promise which resolves to a single message. We are just making an array with all those promises first, and after using `Promise.all()` that array will hold the actual messages you want. – Sergio Baidon Sep 27 '17 at 14:16
This is what getMessage resolves: `resolve({ subject: theSubject, from: theFrom});` I find it hard to grasp how the whole getUnreadMails() function should return an array with all the subjects/froms. – ffritz Sep 27 '17 at 14:19

Array undefined after resolving it after for loop

5 Answers5