Skip to content
Permalink
Browse files

feat($http): remove deprecated callback methods: `success()/error()`

Closes #15157

BREAKING CHANGE:

`$http`'s deprecated custom callback methods - `success()` and `error()` - have been removed.
You can use the standard `then()`/`catch()` promise methods instead, but note that the method
signatures and return values are different.

`success(fn)` can be replaced with `then(fn)`, and `error(fn)` can be replaced with either
`then(null, fn)` or `catch(fn)`.

Before:

```js
$http(...).
  success(function onSuccess(data, status, headers, config) {
    // Handle success
    ...
  }).
  error(function onError(data, status, headers, config) {
    // Handle error
    ...
  });
```

After:

```js
$http(...).
  then(function onSuccess(response) {
    // Handle success
    var data = response.data;
    var status = response.status;
    var statusText = response.statusText;
    var headers = response.headers;
    var config = response.config;
    ...
  }, function onError(response) {
    // Handle error
    var data = response.data;
    var status = response.status;
    var statusText = response.statusText;
    var headers = response.headers;
    var config = response.config;
    ...
  });

// or

$http(...).
  then(function onSuccess(response) {
    // Handle success
    var data = response.data;
    var status = response.status;
    var statusText = response.statusText;
    var headers = response.headers;
    var config = response.config;
    ...
  }).
  catch(function onError(response) {
    // Handle error
    var data = response.data;
    var status = response.status;
    var statusText = response.statusText;
    var headers = response.headers;
    var config = response.config;
    ...
  });
```

**Note:**
There is a subtle difference between the variations showed above. When using
`$http(...).success(onSuccess).error(onError)` or `$http(...).then(onSuccess, onError)`, the
`onError()` callback will only handle errors/rejections produced by the `$http()` call. If the
`onSuccess()` callback produces an error/rejection, it won't be handled by `onError()` and might go
unnoticed. In contrast, when using `$http(...).then(onSuccess).catch(onError)`, `onError()` will
handle errors/rejections produced by both `$http()` _and_ `onSuccess()`.
  • Loading branch information
gkalpak committed Sep 19, 2016
1 parent fb66341 commit b54a39e2029005e0572fbd2ac0e8f6a4e5d69014
Showing with 95 additions and 331 deletions.
  1. +0 −45 docs/content/error/$http/legacy.ngdoc
  2. +0 −60 src/ng/http.js
  3. +2 −2 src/ng/sce.js
  4. +93 −224 test/ng/httpSpec.js

This file was deleted.

@@ -9,11 +9,6 @@ var JSON_ENDS = {
};
var JSON_PROTECTION_PREFIX = /^\)\]\}',?\n/;
var $httpMinErr = minErr('$http');
var $httpMinErrLegacyFn = function(method) {
return function() {
throw $httpMinErr('legacy', 'The method `{0}` on the promise returned from `$http` has been disabled.', method);
};
};

function serializeValue(v) {
if (isObject(v)) {
@@ -346,30 +341,6 @@ function $HttpProvider() {
return useApplyAsync;
};

var useLegacyPromise = true;
/**
* @ngdoc method
* @name $httpProvider#useLegacyPromiseExtensions
* @description
*
* Configure `$http` service to return promises without the shorthand methods `success` and `error`.
* This should be used to make sure that applications work without these methods.
*
* Defaults to true. If no value is specified, returns the current configured value.
*
* @param {boolean=} value If true, `$http` will return a promise with the deprecated legacy `success` and `error` methods.
*
* @returns {boolean|Object} If a value is specified, returns the $httpProvider for chaining.
* otherwise, returns the current configured value.
**/
this.useLegacyPromiseExtensions = function(value) {
if (isDefined(value)) {
useLegacyPromise = !!value;
return this;
}
return useLegacyPromise;
};

/**
* @ngdoc property
* @name $httpProvider#interceptors
@@ -503,14 +474,6 @@ function $HttpProvider() {
* $httpBackend.flush();
* ```
*
* ## Deprecation Notice
* <div class="alert alert-danger">
* The `$http` legacy promise methods `success` and `error` have been deprecated.
* Use the standard `then` method instead.
* If {@link $httpProvider#useLegacyPromiseExtensions `$httpProvider.useLegacyPromiseExtensions`} is set to
* `false` then these methods will throw {@link $http:legacy `$http/legacy`} error.
* </div>
*
* ## Setting HTTP Headers
*
* The $http service will automatically add certain HTTP headers to all requests. These defaults
@@ -1000,29 +963,6 @@ function $HttpProvider() {
promise = chainInterceptors(promise, responseInterceptors);
promise = promise.finally(completeOutstandingRequest);

if (useLegacyPromise) {
promise.success = function(fn) {
assertArgFn(fn, 'fn');

promise.then(function(response) {
fn(response.data, response.status, response.headers, config);
});
return promise;
};

promise.error = function(fn) {
assertArgFn(fn, 'fn');

promise.then(null, function(response) {
fn(response.data, response.status, response.headers, config);
});
return promise;
};
} else {
promise.success = $httpMinErrLegacyFn('success');
promise.error = $httpMinErrLegacyFn('error');
}

return promise;


@@ -612,8 +612,8 @@ function $SceDelegateProvider() {
* .controller('AppController', ['$http', '$templateCache', '$sce',
* function AppController($http, $templateCache, $sce) {
* var self = this;
* $http.get('test_data.json', {cache: $templateCache}).success(function(userComments) {
* self.userComments = userComments;
* $http.get('test_data.json', {cache: $templateCache}).then(function(response) {
* self.userComments = response.data;
* });
* self.explicitlyTrustedHtml = $sce.trustAsHtml(
* '<span onmouseover="this.textContent=&quot;Explicitly trusted HTML bypasses ' +

4 comments on commit b54a39e

@bradw2k

This comment has been minimized.

Copy link
Contributor

@bradw2k bradw2k replied Jan 22, 2019

This appears to be a completely unnecessary breaking change.

@gkalpak

This comment has been minimized.

Copy link
Member Author

@gkalpak gkalpak replied Jan 22, 2019

It is standard practice to eventually remove deplrecated APIs after some time. It prevents people from using these inferior APIs, removes unnecessary code and reduces maintainance cost.

This particular change has minimal impact for the last two points, but significantly contributes to the first one.

@bradw2k

This comment has been minimized.

Copy link
Contributor

@bradw2k bradw2k replied Jan 23, 2019

It appears to have been unnecessary. And Narretz even said "We rarely removed deprecated features." The obvious reason to minimize breaking changes is to ... not break the applications which use the API, when there is no compelling reason to do so. Something to think about.

@gkalpak

This comment has been minimized.

Copy link
Member Author

@gkalpak gkalpak replied Jan 23, 2019

Hm...we have removed several deprecated features. (Off the top of my head: Global controllers, auto-unwrapping of promises, angular.xyz() helpers (e.g. lowercase/uppercase), ngScenario, several ngTouch services/directives, $cookieStore, $compileProvider.preAssignBindingsEnabled(), etc.)

As mentioned, it is standard practice. Especially in this case, I feel strongly that removing the old APIs was benefitial to users. They had been deprecated for a while (so people had plenty of time to plan their migration), the migration was easy enough and the old APIs had subtle caveats that could catch people off-guard (especially with the standardization of Promises).

The obvious reason to minimize breaking changes is to ... not break the applications which use the API, when there is no compelling reason to do so. Something to think about.

I'll try to think about it, but can't promise anything 😛 ☮️

(Agreeing on disagreeing is also fine, btw 😉)

Please sign in to comment.
You can’t perform that action at this time.