Why Are Callbacks from Promise '.Then' Methods an Anti-Pattern

Why are Callbacks from Promise `.then` Methods an Anti-Pattern

The code can be re-factored as follows:

app.controller('tokenCtrl', function($scope, tokenService) {
    tokenService.getTokens.then ( callbackFn(tokens) {
        $scope.tokens = tokens;
    });
});

app.factory('tokenService', function($http) {
    var getTokens = function() {
        //return promise
        return $http.get('/api/tokens').then (function onFulfilled(response) {
                //return tokens
                return response.data;
            }
        );
    };

    return {
        getTokens: getTokens
    };
});

By having the service return a promise, and using the .then method of the promise, the same functionality is achieved with the following benefits:

• The promise can be saved and used for chaining.

• The promise can be saved and used to avoid repeating the same $http call.

• Error information is retained and can be retrieved with the .catch method.

• The promise can be forwarded to other clients.

JavaScript Promises confusion anti-pattern?

export const myExample = (payload) => {
  return new Promise((resolve, reject) => {})
}

should only be used to convert code that is not promise based but returns the result asynchronously into a Promise.

export const myExample = (payload) => {
  return new Promise(async (resolve, reject) => {})
}

Is an anty pattern async already makes a function to return a Promise, and you break the promise chaining here.

export const myExample = async (payload) => {
  return new Promise((resolve, reject) => {})
}

Same as the first one new Promise should only be used to convert code that is not promise based but returns the result asynchronously into a Promise. If async can be committed depends on the other code within that function. But it would be better that if you need to use new Promise((resolve, reject) => {}) that the enclosing function only contains and returns that new Promise like in your first example.

also if that's the case, should I just return from function which will be same as resolve, and if I throw Error will be reject, so it would look like that ?

yes

What is the explicit promise construction antipattern and how do I avoid it?

The deferred antipattern (now explicit-construction anti-pattern) coined by Esailija is a common anti-pattern people who are new to promises make, I've made it myself when I first used promises. The problem with the above code is that is fails to utilize the fact that promises chain.

Promises can chain with .then and you can return promises directly. Your code in getStuffDone can be rewritten as:

function getStuffDone(param){
    return myPromiseFn(param+1); // much nicer, right?
}

Promises are all about making asynchronous code more readable and behave like synchronous code without hiding that fact. Promises represent an abstraction over a value of one time operation, they abstract the notion of a statement or expression in a programming language.

You should only use deferred objects when you are converting an API to promises and can't do it automatically, or when you're writing aggregation functions that are easier expressed this way.

Quoting Esailija:

This is the most common anti-pattern. It is easy to fall into this when you don't really understand promises and think of them as glorified event emitters or callback utility. Let's recap: promises are about making asynchronous code retain most of the lost properties of synchronous code such as flat indentation and one exception channel.

Promises for promises that are yet to be created without using the deferred [anti]pattern

Promises for promises that are yet to be created

…are easy to build by chaining a then invocation with the callback that creates the promise to a promise represents the availability to create it in the future.

If you are making a promise for a promise, you should never use the deferred pattern. You should use deferreds or the Promise constructor if and only if there is something asynchronous that you want to wait for, and it does not already involve promises. In all other cases, you should compose multiple promises.

When you say

When the API call is queued, the promise for the network request would be created at some point in the future

then you should not create a deferred that you can later resolve with the promise once it is created (or worse, resolve it with the promises results once the promise settles), but rather you should get a promise for the point in the future at which the network reqest will be made. Basically you're going to write

return waitForEndOfQueue().then(makeNetworkRequest);

and of course we're going to need to mutate the queue respectively.

var queue_ready = Promise.resolve(true);
function callAPI(params) {
  var result = queue_ready.then(function(API_available) {
    return doRealNetRequest(params);
  });
  queue_ready = result.then(function() {
    return true;
  });
  return result;
}

This has the additional benefit that you will need to explicitly deal with errors in the queue. Here, every call returns a rejected promise once one request failed (you'll probably want to change that) - in your original code, the queue just got stuck (and you probably didn't notice).

The second case is a bit more complicated, as it does involve a setTimeout call. This is an asynchronous primitive that we need to manually build a promise for - but only for the timeout, and nothing else. Again, we're going to get a promise for the timeout, and then simply chain our API call to that to get the promise that we want to return.

function TimeoutQueue(timeout) {
  var data = [], timer = 0;
  this.promise = new Promise(resolve => {
    this.renew = () => {
      clearTimeout(timer);
      timer = setTimeout(resolve, timeout);
    };
  }).then(() => {
    this.constructor(timeout); // re-initialise
    return data;
  });
  this.add = (datum) => {
    data.push(datum);
    this.renew();
    return this.promise;
  };
}

var queue = new TimeoutQueue(10);
function callAPI2(data) {
  return queue.add(data).then(callAPI);
}

You can see here a) how the debouncing logic is completely factored out of callAPI2 (which might not have been necessary but makes a nice point) and b) how the promise constructor only concerns itself with the timeout and nothing else. It doesn't even need to "leak" the resolve function like a deferred would, the only thing it makes available to the outside is that renew function which allows extending the timer.

Is it an anti-pattern to use async/await inside of a new Promise() constructor?

You're effectively using promises inside the promise constructor executor function, so this the Promise constructor anti-pattern.

Your code is a good example of the main risk: not propagating all errors safely. Read why there.

In addition, the use of async/await can make the same traps even more surprising. Compare:

let p = new Promise(resolve => {  ""(); // TypeError  resolve();});
(async () => {  await p;})().catch(e => console.log("Caught: " + e)); // Catches it.

Is this a Deferred Antipattern ?

Is this a “Deferred Antipattern”?

Yes, it is. 'Deferred anti-pattern' happens when a new redundant deferred object is created to be resolved from inside a promise chain. In your case you are using $q to return a promise for something that implicitly returns a promise. You already have a Promise object($http service itself returns a promise), so you just need to return it!

Here's the super simple example of what a service, with a deferred promise and one with antipattern look like,

This is anti-pattern

app.factory("SomeFactory",['$http','$q']){
    return {
        getData: function(){
            var deferred = $q.defer();            
            $http.get(destinationFactory.url)
              .then(function (response) {        
                 deferred.resolve(response.data);
            })
              .catch(function (error) {
                deferred.reject(error);
            });            
            return deferred.promise;
        }
     }
}])

This is what you should do

app.factory("SomeFactory",['$http']){
    return {
        getData: function(){
           //$http itself returns a promise 
            return $http.get(destinationFactory.url);
        }
}

while both of them are consumed in the same way.

this.var = SomeFactory.getData()
    .then(function(response) {
        //some variable = response;
    },function(response) {
        //Do error handling here
});

There's nothing wrong with either examples(atleast syntactically)..but first one is redundant..and not needed!

Hope it helps :)

---

Related Topics