6

I have a function that gets's an object passed into it, with key and data that is an array. I have to call the API to get additional information, which in turn are added back into the object and the whole object is returned.

My first approach was incorrect because I was trying to pass the data out of the .then(), but that was wrong practice.

function asignChecklistItems(taskArray) { 
    // get all the people's tasks passed in
    return new Promise(function(resolve, reject) {

        var promises = []

        // Task Array: {"Person":[tasks,tasks,tasks]}

        for (var person in taskArray) {
            var individualTaskPerson = taskArray[person]
            // get the person's tasks

            for (var individualTask in individualTaskPerson) {

                // here we get each individual task
                var task = individualTaskPerson[individualTask]

                // Check if the checklist is present, the .exists is a bool 
                if (task.checklist.exists === true) {
                //Here we push the promise back into the promise array
                // retrieve is the async function 
                     promises.push( retrieveCheckListItems(task.checklist.checklistID)
                            .then(checklistItems => {
                                var complete = []
                                var incomplete = []
                                const items = checklistItems["checkItems"];
                                for (var each in items) {
                                    const item = items[each]
                                    if (item["state"] === "complete") {
                                        complete.push(item["name"])
                                    } else {
                                        incomplete.push(item["name"])
                                    }
                                }

                                task.checklist.completeItems.push(complete)
                                task.checklist.incompleteItems.push(incomplete)
                                return taskArray // used to be: resolve(taskArray) See **EDIT**
                            })
                            .catch(err => {
                          logError(err)
                                reject(err)
                            })
             )                

                    } else {
              // There's no checklist
            }
                }
            }
        Promise.all(promises).then(function(x){
              // Here when checked, all the items are pushed to the last person inside the array. 
              // Eg. {PersonA:[tasks],PersonB:[tasks],PersonC:[tasks]}
              // All of the complete and incomplete items are added to Person C for some reason
              resolve(taskArray)

        })

        })
    }

I've tried many approaches, returning the entire promise, trying to return from within the promise (which didn't work because it's not allowed), and trying to run the async code earlier, and moving the for loops into the promise. None of them worked, this is the closest, where it returns it for PersonC.

This was mostly based on other SO questions such as this one, which showed how to use Promise.All.

Is this the preoper way of calling a promise (async function) for each element of a for loop?

EDIT:

Another mistake that was in the code, is that if there is a promise insde a promise, such as the retrieveCheckListItems inside of the asignCheckListItems, it shouldn't resolve itself, but it should return the value. I updated the code to reflect that, based on the working production code.

Apparently another problem I was

Julian E.
  • 4,687
  • 6
  • 32
  • 49
  • Promise.all(promises) is working?What browsers do you support? – Alex Nikulin Nov 16 '17 at 10:55
  • This is running in node. Yeah, it gets them, but adds it to personC – Julian E. Nov 16 '17 at 10:58
  • `Promise.all` is fine, but avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! – Bergi Nov 16 '17 at 14:08

1 Answers1

2

You are executing task.checklist.completeItems.push(complete) in the retrieveCheckListItems .then, which means the code is asynchronous.

At the same time, var task is instead being assigned in a for...in loop, which means that by the time your .then is triggered, the for...in iteration will be complete and task will be the last assigned object.

Note that var variables have a function scope, which basically means that your code is equivalent to:

function asignChecklistItems(taskArray) { 
    // get all the people's tasks passed in
    return new Promise(function(resolve, reject) {

        var promises = []
        var individualTaskPerson;
        var task;

        ...

Fix it by:

  1. Either changing var task to let task (if you are using ES6). This then creates the variable within each for loop, rather than within the enclosing function.

    or

  2. By replacing

    for (var individualTask in individualTaskPerson) {
    
      // here we get each individual task
      var task = individualTaskPerson[individualTask]
    

    with

    Object.keys(individualTaskPerson).forEach(function(individualTask) {
        var task = individualTaskPerson[individualTask];  
    
         ...
    });
    

   

Do the same for the other for...in loops and variables.

Community
  • 1
  • 1
Maluen
  • 1,753
  • 11
  • 16
  • 1
    Thank you, I'll take a look now. I'm not sure I understand the part that says that it's the same as the code you provided... That is the code in the way I wrote it – Julian E. Nov 16 '17 at 13:08
  • 1
    @JulianE. Look at how the `var` declarations were moved at the beginning of the `new Promise` function. This means that in the for...in iterations you are actually assigning a value to the same `task` variable, instead of creating a new one every time. – Maluen Nov 16 '17 at 13:28
  • 1
    Ah, thank you very much! I think I understood it now. So `let` creates it locally in the code block, and `var` creates it globally, and it was just resigning it? – Julian E. Nov 16 '17 at 13:31
  • 1
    Yes pretty much, not really globally, but relative to the current function. – Maluen Nov 16 '17 at 13:34