106

I have implemented the $q.all in angularjs, but I can not make the code work. Here is my code :

UploadService.uploadQuestion = function(questions){

        var promises = [];

        for(var i = 0 ; i < questions.length ; i++){

            var deffered  = $q.defer();
            var question  = questions[i]; 

            $http({

                url   : 'upload/question',
                method: 'POST',
                data  : question
            }).
            success(function(data){
                deffered.resolve(data);
            }).
            error(function(error){
                deffered.reject();
            });

            promises.push(deffered.promise);
        }

        return $q.all(promises);
    }

And here is my controller which call the services:

uploadService.uploadQuestion(questions).then(function(datas){

   //the datas can not be retrieved although the server has responded    
}, 
function(errors){ 
   //errors can not be retrieved also

})

I think there is some problem setting up $q.all in my service.

Mel
  • 5,837
  • 10
  • 37
  • 42
themyth92
  • 1,743
  • 2
  • 17
  • 23

3 Answers3

225

In javascript there are no block-level scopes only function-level scopes:

Read this article about javaScript Scoping and Hoisting.

See how I debugged your code:

var deferred = $q.defer();
deferred.count = i;

console.log(deferred.count); // 0,1,2,3,4,5 --< all deferred objects

// some code

.success(function(data){
   console.log(deferred.count); // 5,5,5,5,5,5 --< only the last deferred object
   deferred.resolve(data);
})
  • When you write var deferred= $q.defer(); inside a for loop it's hoisted to the top of the function, it means that javascript declares this variable on the function scope outside of the for loop.
  • With each loop, the last deferred is overriding the previous one, there is no block-level scope to save a reference to that object.
  • When asynchronous callbacks (success / error) are invoked, they reference only the last deferred object and only it gets resolved, so $q.all is never resolved because it still waits for other deferred objects.
  • What you need is to create an anonymous function for each item you iterate.
  • Since functions do have scopes, the reference to the deferred objects are preserved in a closure scope even after functions are executed.
  • As #dfsq commented: There is no need to manually construct a new deferred object since $http itself returns a promise.

Solution with angular.forEach:

Here is a demo plunker: http://plnkr.co/edit/NGMp4ycmaCqVOmgohN53?p=preview

UploadService.uploadQuestion = function(questions){

    var promises = [];

    angular.forEach(questions , function(question) {

        var promise = $http({
            url   : 'upload/question',
            method: 'POST',
            data  : question
        });

        promises.push(promise);

    });

    return $q.all(promises);
}

My favorite way is to use Array#map:

Here is a demo plunker: http://plnkr.co/edit/KYeTWUyxJR4mlU77svw9?p=preview

UploadService.uploadQuestion = function(questions){

    var promises = questions.map(function(question) {

        return $http({
            url   : 'upload/question',
            method: 'POST',
            data  : question
        });

    });

    return $q.all(promises);
}
Ilan Frumer
  • 32,059
  • 8
  • 70
  • 84
  • 14
    Good answer. One addition: no need to construct new deferred since $http itself returns promise. So it can be shorter: http://plnkr.co/edit/V3gh7Roez8WWl4NKKrqM?p=preview – dfsq Jan 24 '14 at 06:29
  • "When you write var deferred= $q.defer(); inside a for loop it's hoisted to the top of the function.". I dont understand this part, can you explain the reason behind it ? – themyth92 Jan 24 '14 at 06:53
  • I know, I would do the same actually. – dfsq Jan 24 '14 at 12:33
  • 4
    Love the use of `map` to build an array of promises. Very simple and concise. – Drumbeg Aug 10 '15 at 13:03
  • Legendary answer! Thanks to your explanation, I can see that since you are pushing the promise returned by the $http call directly into your array of promises, you no longer need a separate scope to save the reference to the unnecessary, intermediary deferred. This means the OP could technically go back to using a simple for loop, although I must admit that using "map" seems to be the most elegant way to create an array of promises in this case. – Niko Bellic Nov 16 '15 at 23:13
  • Great answer. Let's say this wasn't a POST but a GET method. How do you iterate over the promises to get data belonging to each promise? – Tisha Mar 16 '16 at 17:32
  • 1
    It should be noted that the declaration is hoisted, but the assignment stays where it is. Also, there is now block-level scoping with the 'let' statement. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let – Spencer Aug 31 '16 at 14:46
37

$http is a promise too, you can make it simpler:

return $q.all(tasks.map(function(d){
        return $http.post('upload/tasks',d).then(someProcessCallback, onErrorCallback);
    }));
Zerkotin
  • 396
  • 3
  • 3
  • 2
    Yes, the accepted answer is just an agglomeration of anti-patterns. – Roamer-1888 Jul 20 '14 at 23:24
  • I think you can leave the `.then()` clause out as the OP wants to do all that in his controller, but the principle is totally correct. – Roamer-1888 Jul 20 '14 at 23:29
  • 2
    of course you can omit the then clause, i added it in case you want to log all the HTTP requests, i always add them and use a global onError callback, to handle all server exceptions. – Zerkotin Jul 21 '14 at 14:04
  • 1
    @Zerkotin you can `throw` from a `.then` in order to both handle it later _and_ expose it to `$exceptionHandler`, which should save you that trouble and a global. – Benjamin Gruenbaum Aug 07 '14 at 06:16
  • Nice. This is essentially the same approach as the accepted answer's last solution/example. – Niko Bellic Aug 23 '16 at 21:50
  • Would `.then` fire for every promise of `e`? How would I get `someCallback` to fire just when all of the promises return? Add it to `$q.all(promises).then(someCallBack)`? – Jimenemex Nov 29 '18 at 22:26
12

The issue seems to be that you are adding the deffered.promise when deffered is itself the promise you should be adding:

Try changing to promises.push(deffered); so you don't add the unwrapped promise to the array.

 UploadService.uploadQuestion = function(questions){

            var promises = [];

            for(var i = 0 ; i < questions.length ; i++){

                var deffered  = $q.defer();
                var question  = questions[i]; 

                $http({

                    url   : 'upload/question',
                    method: 'POST',
                    data  : question
                }).
                success(function(data){
                    deffered.resolve(data);
                }).
                error(function(error){
                    deffered.reject();
                });

                promises.push(deffered);
            }

            return $q.all(promises);
        }
Davin Tryon
  • 66,517
  • 15
  • 143
  • 132
  • This only returns an array of deffered objects, I checked it. – Ilan Frumer Jan 23 '14 at 17:25
  • I don't know what he says, only what the console says, You can see it's not working: http://plnkr.co/edit/J1ErNncNsclf3aU86D7Z?p=preview – Ilan Frumer Jan 23 '14 at 17:34
  • 4
    Also the documentation says clearly that `$q.all` gets promises not deferred objects. The real problem of the OP is with scoping and because that only the last deferred is getting resolved – Ilan Frumer Jan 23 '14 at 17:37
  • Ilan, thanks for disentangling `defer` objects and `promises`. You fixed my `all()` issue as well. – Ross Rogers Jun 06 '14 at 16:43
  • the problem was resolved in 2 answers, the problem is scoping or variable hoisting, whatever you want to call it. – Zerkotin Aug 12 '14 at 11:37