$http .success/.error return original promise, dissimilar from how .then works. Clarify in docs?

[glebec]

Hello. $http returns a normal promise for a server response object (with data, status, headers etc.). As a normal promise, returning values from a handler in .then results in a new exported promise for the value (or assimilation, if the return value is a promise, or rejection in case of error, etc.). That is all well and good.

For convenience/abstraction, AngularJS provides two custom methods on promises returned by $http: .success and .error. Each takes a single handler with the data, status etc. as parameters.

The problem is that people familiar with promises will likely try to chain off of .success, perhaps by transforming and returning new values or new promises. However, .success does not return a new promise; rather, it returns the original promise:

angular.module('promisesApp')
  .controller('MainCtrl', function ($http, $log) {

    $http.get('/api/things')
      .success(function (data) {
        $log.debug('in success:', data); // these are the things from the API
        return 'a new value';
      })
      .then(function (data) {
        $log.debug('in chained .then:', data);
        // data is the original server response object, NOT 'a new value'
        // data.data are the things from the API
      });

  });

If you were trying to make .success chainable in the same way .then is, it would be easy to change in the AngularJS source, and indeed I had started work on a pull request:

// in theory, could change this:
promise.success = function(fn) {
  promise.then(function(response) {
    fn(response.data, response.status, response.headers, config);
  });
  return promise;
};
// to this:
promise.success = function(fn) {
  return promise.then(function(response) {
    return fn(response.data, response.status, response.headers, config);
  });
};

However, I then realized that this would mean you could not attach both .success and .error to a single promise. The reason you can write this code:

$http.get('api/things')
  .success( successHandler )
  .error( errorHandler )

…is because .success and .error return the original promise, not a new promise representing the result of the previous method.

This leaves me in a bit of a quandary. The recommendation must therefore be that .success and .error are only suitable as "end of the road" promise consumers, when you do not intend to do any more post-processing / chaining. If that is the case, I think the AngularJS docs should reflect this, and make it explicitly clear that chaining off of .success/.error does not work in the normal promises fashion, since the original promise is returned.

What do you think?

[petebacondarwin]

IMO .success and .error were a bad bit of API design in the first place. This issue highlights a number of situations where developers get confused because they either expect .success and .error to work the same way as .then or vice versa.
In a perfect world I would rather just ditch these $http specific "promises". Instead we could encourage developers to use the standard $q promise API .then and .catch. There is very little benefit IMO in working with explicit parameters over working with the response object.

[glebec]

I'm starting to feel the same way. At first I considered .success to be nice because all I ever wanted was the data, but since discovering this behavior I realized that:

A) I was not bothering to attach .error (bad!)
B) I couldn't use .success in a promise-y way, returning values/promises from handlers.

Given those points, in the future I will personally be avoiding .success in my own code, sticking exclusively to .then. Still, the current code exists, and if it continues to exist I think there should be a warning or info box in the docs regarding this issue.

[pkozlowski-opensource]

Yeh, I'm also quite in favour of using promise-based API over .error / .success convenience callback-based counterparts. I can see how those callback-based shortcuts can be nice (especially to people not familiar with the promise API) but we should make it clear that .error / .success are not-promise based and people shouldn't have any promise-related expectiations while using callback-style APIs.

I don't think removing .error / .success is a realistic option at this point but I'm not sure how to properly document those things...

[pkozlowski-opensource]

If anyone can think of good wording for the doc update I would be happy to merge.

[glebec]

I'll have a go at adding a line to the docs and making a pull request (when I have a moment to go through the 25-point contribution checklist, that is). Expect to see the request this week.

[tregusti]

Just ran into this myself. Did a nice fiddle for it, but then I found this issue. I would really like to either remove the methods or fix them. Adding to docs feels like the easy way out.

[petebacondarwin]

I think we should be deprecating .error and .success in 1.4.
It does not require a deep understanding of promises to go from simply writing:

$http.get(...)
  .success(function(data, ...) {
    ...
  })
  .error(function(data, ...) {
    ...
  });

to writing

$http.get(...).then(
  function success(response) {
    var data = response.data;
    ...
  },
  function error(response) {
    var data = response.data;
  });

while it does require a deepish understanding of promise to understand why .success and .error to function exactly like .then. The extra verbosity is worth it in my opinion.

[pkozlowski-opensource]

@petebacondarwin would marking them as depreciated a first step of removing them in 1.5?

[petebacondarwin]

Exactly

[likaiwalkman]

@glebec Long time ago, I'm confused with success invocation too. It breaks the specification promise must be resolved exactly once.