Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

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

Closed
glebec opened this issue Dec 17, 2014 · 10 comments
Closed

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

glebec opened this issue Dec 17, 2014 · 10 comments
Labels
component: $http type: docs
Milestone
Backlog - won't fix

Comments

@glebec
Copy link

glebec commented Dec 17, 2014

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
Copy link
Member

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
Copy link
Author

glebec commented Dec 17, 2014

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
Copy link
Member

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
Copy link
Member

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

@pkozlowski-opensource pkozlowski-opensource added this to the Backlog milestone Dec 18, 2014
@glebec
Copy link
Author

glebec commented Dec 18, 2014

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
Copy link

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
Copy link
Member

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
Copy link
Member

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

@petebacondarwin
Copy link
Member

Exactly

Closed
@gkalpak gkalpak mentioned this issue Feb 18, 2015
Closed
Closed
@gkalpak gkalpak mentioned this issue May 29, 2015
Closed
PinkyJie pushed a commit to PinkyJie/generator-aio-angular that referenced this issue Jun 16, 2015
refactor promise
18de9f0 
1. do not use success/error callback for $http cause they don't follow the promise standard
2. do not use defered API cause it's a duplicated promise

referenced link:
angular/angular.js#10508
http://pouchdb.com/2015/05/18/we-have-a-problem-with-promises.html
https://github.com/petkaantonov/bluebird/wiki/Promise-anti-patterns#the-deferred-anti-pattern
@petebacondarwin petebacondarwin mentioned this issue Jun 16, 2015
Closed
netman92 pushed a commit to netman92/angular.js that referenced this issue Aug 8, 2015
@realityking @netman92
feat($httpProvider): add 'useLegacyPromiseExtensions' configuration
7f2d8cc 
The legacy methods, `success` and `error`, have been deprecated.

Set this to `false` to cause `$http` to throw an error if these methods are
used in the application.

For now it defaults to `true`. In a future release we will remove these
methods altogether.

DEPRECATION NOTICE:

The legacy methods 'success' and 'error' on promises returned by $http
are now deprecated.

Closes angular#12112
Closes angular#10508
ggershoni pushed a commit to ggershoni/angular.js that referenced this issue Sep 29, 2015
@realityking @petebacondarwin
feat($httpProvider): add 'useLegacyPromiseExtensions' configuration
377ac07 
The legacy methods, `success` and `error`, have been deprecated.

Set this to `false` to cause `$http` to throw an error if these methods are
used in the application.

For now it defaults to `true`. In a future release we will remove these
methods altogether.

DEPRECATION NOTICE:

The legacy methods 'success' and 'error' on promises returned by $http
are now deprecated.

Closes angular#12112
Closes angular#10508
@likaiwalkman
Copy link

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
component: $http type: docs
Projects
None yet
Milestone
Backlog - won't fix
Development

No branches or pull requests
5 participants
@petebacondarwin @pkozlowski-opensource @tregusti @likaiwalkman @glebec