0

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.

Improve this question
5
  • 1
    getMessage looks like it is asynchron. this wont work, because you're calling resolve before the callback of getMessage is called Commented Sep 27, 2017 at 13:42
  • developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/… Commented Sep 27, 2017 at 13:42
  • @JoshuaK Exactly what I think too. How can this be done correct? Commented Sep 27, 2017 at 13:42
  • Avoid the Promise constructor antipattern! And always use Promise.all on loops Commented Sep 27, 2017 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. Commented Sep 27, 2017 at 13:50

5 Answers 5

Reset to default
3

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

I really like this approach, thanks! In the short version, where does the mailArray then actually get filled/resolved?
.then(Promise.all) won't work, it would need to be bound to Promise. Better move it inside the arrow function.
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 updated the answer to address the question. Hope this makes it clearer. Also fixed the error with .then(Promise.all) that Bergi mentioned.
0

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

Improve this answer

2 Comments

You still will need to avoid the Promise constructor antipattern!
Good shout. I was looking for a quick solution but you're right - we should avoid anti-patterns
0 
            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);
        }
        )
}
Improve this answer

Comments

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!

Improve this answer

5 Comments

I see, this makes sense now, thanks! The resolve part is missing in Thomas answer in the short part below I think.
Actually I think his answer is great, I'm pretty should it would work as well
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.
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.
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.
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)))

}
Improve this answer

1 Comment

.then(Promise.all) won't work, it would need to be bound to Promise. Better move it inside the arrow function.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.