Review of Promise object in Angular

Question

Ok i have the following code

Load: function (urlInfo, moduleInfo) {

    return (function() {

        var paramsObj = CheckParams(urlInfo.params);

        if (paramsObj != null)
            return $http.get(urlInfo.url, { params: paramsObj, cache: $templateCache });
        else
            return $http.get(urlInfo.url, { cache: $templateCache });

    }()).then(this.successFn, this.errorFn);
},

successFn: function (response) { 
    if (response) {
        return response;
    } else {
        // invalid response
        return $q.reject(response);
    }
},

errorFn: function (response) { 
    // something went wrong
    return $q.reject(response);
},

I think the above code has problems because it not use the promise and don't use the deferred object and also don't make the resolve of the object and i think the code must be reviewevd like that:

GetData: function (urlInfo) {
    return  function () {

        var deferred = $q.defer();
        var paramsObj = CheckParams(urlInfo.params);

        if (paramsObj != null){
            $http.get(urlInfo.url, { params: paramsObj })
            .success(function (data, status, headers, config) {
                //resolve the promise
                deferred.resolve(data);  //#1
            })
            //if request is not successful
            .error(function (data, status, headers, config) {
                //reject the promise
                deferred.reject('ERROR');
            });
        }
        return deferred.promise;
    }()).then(function (resolve) {
        return resolve;
    }, function (reject) {
        return reject;
    });
}

because i m not expert you can tell me what are the problems that can come out using the first code (if there are problems)

In fact [your first snippet is much better than the second](http://stackoverflow.com/q/23803743/1048572). — Bergi, Dec 09 '15 at 12:10
Those `successFn` and `errorFn` methods are a bit odd. Do you expect them to be overwritten (e.g. through subclassing)? If not, then just use inline function expressions — Bergi, Dec 09 '15 at 12:11
If I want synchronised calls how I have to work with the first snippet ? This will not bring to mistakes in code ? I can chain the calls without use the defer and the promise ? — FrankTan, Dec 09 '15 at 12:57
"*But the first snippet don't use the defer*" - that's the point. You can return promises even without using `defer()`, and you should do so. Please read the linked question and its answers. — Bergi, Dec 09 '15 at 13:10
Btw, neither of your two snippets actually work, as you are calling `.then(…)` on a `(function(){…})`. Did you mean to make that an IIFE? — Bergi, Dec 09 '15 at 13:12
i read you link and the user "thefourtheye" says to use the Promise.resolve function in the first snippet is not used this! — FrankTan, Dec 09 '15 at 13:12
First snipped don't use anywhere the resolve this is the problem .... that i try to show you — FrankTan, Dec 09 '15 at 13:13
The `Promise.resolve` function that @thefourtheye mentions is something different than the `deferred.resolve()` call you think it is. The promise that `.then(…)` returns is **resolved internally** by the code of `then` so that you don't have to do it explicitly. Only because you don't see the word "resolve" anywhere doesn't mean that the promise won't get fulfilled. — Bergi, Dec 09 '15 at 13:19
I understand because this is a single Call, but i have more calls and i need the referement to one of the call in this way i lost it ... for example in this case... http://stackoverflow.com/questions/31020497/q-defer-then-of-the-then — FrankTan, Dec 09 '15 at 13:22
What do you mean by "*i have more calls and i need the referement to one of the call in this way i lost it*"? Without code, this just sounds like [How do I access previous promise results in a .then() chain?](http://stackoverflow.com/q/28250680/1048572) (and there's no reason to use deferreds in that case either) — Bergi, Dec 09 '15 at 13:24
ok last question is i read that using Promise.resolve is the right way "pattern" in the link you showed me ... why i could not use that way ? — FrankTan, Dec 09 '15 at 13:29
No, `Promise.resolve(x)` is only the right pattern to replace `new Promise((s,e) => { x.then(s, e); })`/`var d=defer(); x.then(d.resolve,d.reject); x.promise`. You'd still chain a proper, regular `.then(…)` call to it if you want to do anything with the data. In your case, it would be `$q.when($http.get(…))` (`$q.when` ~ `Promise.resolve` for angular promises), but that's rather pointless as `$http.get` already returns an angular promise. — Bergi, Dec 09 '15 at 13:37
@TomG If I read the question and comments correctly it's not working as intended. Broken code is strictly off-topic at Code Review. — Mast, Dec 09 '15 at 15:55
Working or not, it would also be closed as "Unclear what you're asking" in either case. — Kaz, Dec 09 '15 at 16:05
@Zak i asked to review the code and comparing the code between the 2 snippet and if there would it generate problems Bergi helped me to understand all behind scene — FrankTan, Dec 09 '15 at 16:58

score 0 · Accepted Answer · answered Dec 09 '15 at 12:17

0

$http service already returned a promise when you call the get method. You could just do return $http.get.... Then in your controller, you will be able to add then, success, error after your method call.

answered Dec 09 '15 at 12:17

Sébastien Ollivier

578
2
10

Sorry the point is not how to use the then. The point is why the first code should work without defer and resolve ? – FrankTan Dec 09 '15 at 12:55
The main point in Sébastien's answer is that "$http service already returned a promise when you call the get method", which is reason enough to qualify the question's first code block as better than the second. – Roamer-1888 Dec 09 '15 at 18:37

Review of Promise object in Angular

1 Answers1