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

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

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Sep 19, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature removal (refactor but with a breaking change).

What is the current behavior? (You can also link to an open issue here)
The promises returned by $http have additions, non-standard methods: success(), error(). They have beed deprecated since v1.4.4 (#12112).
Although there was a switch to disable them ($httpProvider.useLegacyPromiseExtensions()), they were enabled by default.

What is the new behavior (if this is a feature change)?
success() and error() are gone. Use the standard then()/catch() promise methods instead.

Does this PR introduce a breaking change?
Yes.

Please check if the PR fulfills these requirements

Other information:

  • Add a note to the v1.5.x docs that the methods/switch will be removed in v1.6.0.

@Narretz
Copy link
Contributor

Narretz commented Sep 20, 2016

Were all docs updated when this feature was deprecated? Otherwise we'd have to check that.

@gkalpak
Copy link
Member Author

gkalpak commented Sep 21, 2016

Good point. I saw there were docs updates in #12112, so I assumed all docs were updated.
It turns out there was one last survivor in $sce's docs 😃

@Narretz
Copy link
Contributor

Narretz commented Sep 23, 2016

LGTM.
Minor note: in the commit message BC notes, you advise using .catch for error handling, but you don't mention the second argument of then (the error handler fn). Is this on purpose?
Did you talk about the consequences in the meeting? We rarely removed deprecated features. What's the consensus on the impact of this change?

@gkalpak
Copy link
Member Author

gkalpak commented Sep 25, 2016

In the commit message BC notes, you advise using .catch for error handling, but you don't mention the second argument of then (the error handler fn). Is this on purpose?

Yes and no. I don't specifically advise using .catch to replace .error. I have a generic statement that you should use then/catch now and I have one example, where .error is replaced by .catch (because in that particular example it is more straight-forward).

Pedantically speaking, .then(onSuccess, onError) is semantically closer to .success(onSuccess).error(onError). But only because .success/.error were bad APIs 😃 What people usually want is .then(onSuccess).catch(onError) (which is not equivalent to .success(...).error(...)).

So, while I didn't mean to explicitly advice people replacing .success(...).error(...) with .then(...).catch(...), I do think it is good practice (i.e. better than .then(..., ...) - unless you know what you're doing).

But I don't feel strongly about it. If you have concerns about the implications of that, I would be happy to discuss 😃

Did you talk about the consequences in the meeting? We rarely removed deprecated features. What's the consensus on the impact of this change?

We didn't talk much about it. We just agreed it is time for this long-deprecated API to go.
The impact would be the same as with most breaking changes afaict: People will need to migrate their apps to not use the removed feature 😃

BTW, it is not true that we haven't removed other deprecated stuff before (don't judge by replace: true - it is a special case 😛 ).

I'll leave this unmerged for the time being and let's revisit on the next meeting to make sure we didn't miss anything.

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()`.
@gkalpak gkalpak force-pushed the refactor-http-remove-deprecated-methods branch from 5027b09 to 462cb12 Compare October 5, 2016 15:37
@gkalpak gkalpak closed this in b54a39e Oct 5, 2016
@gkalpak gkalpak deleted the refactor-http-remove-deprecated-methods branch October 5, 2016 16:04
ellimist pushed a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017
Closes angular#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()`.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants