feature($http): allow differentiation between Request Error, Abort and Timeout #15847

frederikprijck · 2017-03-22T11:43:34Z

This is a continuation of #15626

frederikprijck · 2017-03-22T11:52:28Z

Travis fails:

20170322 114633.523 [3058] Starting secure remote tunnel VM...
20170322 114638.197 [3058] Secure remote tunnel VM provisioned.
20170322 114638.197 [3058] Tunnel ID: 92dd4c9c55354e3299ba2f05f924ab85
20170322 114638.553 [3058] GET https://saucelabs.com/rest/v1/angular-ci/tunnels/92dd4c9c55354e3299ba2f05f924ab85: HTTP response code indicated failure.
20170322 114638.553 [3058] Response: {"message": "API rate limit exceeded for public:angular-ci. See rate-limiting section in our API documentation."}.
20170322 114638.553 [3058] Error bringing up tunnel VM.
20170322 114638.553 [3058] Cleaning up.
20170322 114638.553 [3058] Removing tunnel 92dd4c9c55354e3299ba2f05f924ab85.

gkalpak · 2017-03-22T13:19:52Z

src/ngMock/angular-mocks.js

    if (angular.isFunction(status)) return status;

    return function() {
      return angular.isNumber(status)
-          ? [status, data, headers, statusText]
+          ? [status, data, headers, statusText, xhrStatus]
          : [200, status, data, headers];


No xhrStatus here?

well, I could put a hardcoded 'success' there. But as 200 is already hardcoded, I was getting confused whether or not xhrStatus needs to be here.

gkalpak · 2017-03-22T13:31:20Z

src/ngMock/angular-mocks.js

@@ -1381,15 +1381,22 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
      function handleResponse() {
        var response = wrapped.response(method, url, data, headers, wrapped.params(url));
        xhr.$$respHeaders = response[2];
-        callback(copy(response[0]), copy(response[1]), xhr.getAllResponseHeaders(),


I wonder why we are coyping stuff that theoretically are primitive (such as status, statusText).

The copy was already there, I added the extra param in the same way. But yes, I was wondering the exact same thing. Maybe there used to be a reason :D

gkalpak · 2017-03-22T13:32:00Z

src/ngMock/angular-mocks.js

@@ -1381,15 +1381,22 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
      function handleResponse() {
        var response = wrapped.response(method, url, data, headers, wrapped.params(url));
        xhr.$$respHeaders = response[2];
-        callback(copy(response[0]), copy(response[1]), xhr.getAllResponseHeaders(),
+        var xhrStatus = copy(response[4]);
+        if (xhrStatus) {


Why the different code-paths?

Apparently, I forgot to push things at home. I've fixed this already. I'll push it in 2 hours (+/-).

This was the quick and dirty check I did in order to stay backwards compatible (and not break 60 tests). But it's a better idea to break and fix them for this change.

So I've reduced it to a single path and fixed all 60+ braking tests.

frederikprijck · 2017-03-22T17:15:43Z

src/ngMock/angular-mocks.js

    if (angular.isFunction(status)) return status;

    return function() {
      return angular.isNumber(status)
-          ? [status, data, headers, statusText]
-          : [200, status, data, headers];
+          ? [status, data, headers, statusText, xhrStatus || (status === 200 ? 'success' : 'error')]


@gkalpak It feels abit odd to be calculating xhrStatus (the above example isn't what it should be yet). I went down the path of an hardcoded 'success' value because of the hardcoded 200 on line 1350. Hardcoding 'success' made me wonder if we should be calculating xhrStatus based on status (if xhrStatus was not provided). But it brings some (unnessecary?) complexity in here.

Maybe it's a good idea to not set any xhrStatus based on the status, apart from the hardcoded 'success' on line 1350, and change this line (1349) to [status, data, headers, statusText, xhrStatus] keeping xhrStatus undefined if not provided, unless status is not a number (then xhrStatus would be 'success'). But this doesn't sound very consistent ... 😬

Since xhrStatus is something that is generated by $httpBackend (i.e. it is not something that comes from the server, I think we should not even make it "trainable". I.e. you should:

Remove the xhrStatus argument from createResponse (and other similar places).

Generate xhrStatus inside createResponse, based on the same rules we use in $httpBackend (i.e. 200 <= status < 300 ? 'success' : 'error').

Update existing tests. Add new tests that verify this new behavior.

Error as of 3xx or 4xx ?

I just realized 'success' is confusing. I was thinking it means something different than it actually does. Maybe we should change it to complete or something.
Then you should change both lines to xhrStatus || 'complete'.

So does that mean that response 300 up to 500 are to be considered not completed?

No, all responses are considered 'complete' ('complete' means that the request completed, i.e. a response was received from the server, regardless of what the status of the response was).

@gkalpak

Then you should change both lines to xhrStatus || 'complete'.

Does it still make sense to use xhrStatus || 'complete' ?

function createResponse(status, data, headers, statusText, xhrStatus) { if (angular.isFunction(status)) return status; return function() { return angular.isNumber(status) ? [status, data, headers, statusText, xhrStatus || 'complete'] : [200, status, data, headers, xhrStatus || 'complete']; }; }

As we're always returning a status code, I guess we can consider it 'complete' in both situations? This would be in line with how this change is currently implemented in$httpBackend (none-mock).

Or is there any need to manually set the xhrStatus to anything else but 'complete' when using .respone() ?

LOL - I contradict myself in these comments: #15847 (comment)
#15847 (comment)
It doesn't make sense to make it configurable imo, so let's remove the xhrStatus argument and make it 'complete' everywhere.

gkalpak · 2017-03-24T10:55:12Z

src/ngMock/angular-mocks.js

    if (angular.isFunction(status)) return status;

    return function() {
      return angular.isNumber(status)
-          ? [status, data, headers, statusText]
-          : [200, status, data, headers];
+          ? [status, data, headers, statusText, xhrStatus || (status === 200 ? 'success' : 'error')]


Since xhrStatus is something that is generated by $httpBackend (i.e. it is not something that comes from the server, I think we should not even make it "trainable". I.e. you should:

Remove the xhrStatus argument from createResponse (and other similar places).

Generate xhrStatus inside createResponse, based on the same rules we use in $httpBackend (i.e. 200 <= status < 300 ? 'success' : 'error').

Update existing tests. Add new tests that verify this new behavior.

katsos · 2017-06-20T08:38:58Z

@frederikprijck do you need help on this one?

…d Timeout Fixes #15924

frederikprijck · 2017-06-20T19:09:56Z

@Katos I did some cleaning up, depending on the review result it might be close to finished. Sorry for taking so long tho!

@gkalpak mind to review it?

Narretz · 2017-06-29T09:16:00Z

@gkalpak I see this is labelled as "needs breaking change". I can't see how this is breaking atm :/

gkalpak · 2017-07-03T19:51:59Z

src/ngMock/angular-mocks.js

@@ -1382,14 +1382,14 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
        var response = wrapped.response(method, url, data, headers, wrapped.params(url));
        xhr.$$respHeaders = response[2];
        callback(copy(response[0]), copy(response[1]), xhr.getAllResponseHeaders(),
-                 copy(response[3] || ''));
+               copy(response[3] || ''), copy(response[4]));


What's with the 7-space indentation? 😕

Well, the linter agrees so ... 🙈 (on a serious note: no clue, I'll get it removed before merging)

gkalpak · 2017-07-03T20:11:56Z

@Narretz, (although not likely) it could break tests that assert an $http promise was resolved with an object containing certain properties. E.g.:

const mySpy = jasmine.createSpy('mySpy');

$httpBackend.whenGET('/foo').respond('bar');
$http.get('/foo').then(mySpy);
$httpBackend.flush();
    
expect(mySpy).toHaveBeenCalledWith({
  data: 'bar',
  status: 200,
  statusText: '',
  headers: jasmine.any(Function),
  config: jasmine.any(Object),
});

This would previously pass, but it will fail with this change because the xhrStatus property will be missing.

I admit the possibility this affects anyone is tiny, but still possible. I added the label, so we can discuss it when the time comes (which is about now 😃).

Narretz · 2017-07-04T10:14:06Z

i think the feature is too useful and the possible breaking change to minor to put this into 1.7. I'd say put it into 1.6

gkalpak · 2017-07-04T11:36:12Z

OK. Let's do this 😃

…rt, timeout Previously, it wasn't possible to tell if an `$http`-initiated XMLHttpRequest was completed normally or with an error or it was aborted or timed out. This commit adds a new property on the `response` object (`xhrStatus`) which allows to defferentiate between the possible statuses. Fixes #15924 Closes #15847

evanjmg · 2021-01-07T14:12:08Z

did this end up in a release somewhere?

frederikprijck · 2021-01-07T14:24:35Z

@evanjmg yes 1.6.6, see: https://github.com/angular/angular.js/blob/master/CHANGELOG.md

googlebot added cla: yes labels Mar 22, 2017

gkalpak reviewed Mar 22, 2017

View reviewed changes

frederikprijck commented Mar 22, 2017

View reviewed changes

gkalpak suggested changes Mar 24, 2017

View reviewed changes

gkalpak added component: $http needs: breaking change needs: docs needs: work type: feature labels Mar 27, 2017

gkalpak added this to the Backlog milestone Mar 27, 2017

frederikprijck mentioned this pull request Apr 19, 2017

Angular $http - handle differently xhr.onabort, xhr.ontimeout and xhr.onerror #15924

Closed

3 tasks

Narretz mentioned this pull request Apr 20, 2017

$http: allow differentiation between Request Error, Abort and Timeout #15626

Closed

3 tasks

feature($http): allow differentiation between Request Error, Abort an…

500a46d

…d Timeout Fixes #15924

frederikprijck closed this Jun 20, 2017

frederikprijck reopened this Jun 20, 2017

Narretz modified the milestones: 1.6.x, Backlog Jun 28, 2017

gkalpak approved these changes Jul 3, 2017

View reviewed changes

frederikprijck added 2 commits July 4, 2017 13:54

fix spaces

2fcbc76

fix spaces, try 2

f258606

gkalpak closed this in e872f0e Jul 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature($http): allow differentiation between Request Error, Abort and Timeout #15847

feature($http): allow differentiation between Request Error, Abort and Timeout #15847

frederikprijck commented Mar 22, 2017 •

edited

Loading

frederikprijck commented Mar 22, 2017 •

edited

Loading

gkalpak Mar 22, 2017

frederikprijck Mar 22, 2017

gkalpak Mar 22, 2017

frederikprijck Mar 22, 2017 •

edited

Loading

gkalpak Mar 22, 2017

frederikprijck Mar 22, 2017 •

edited

Loading

frederikprijck Mar 22, 2017 •

edited

Loading

gkalpak Mar 24, 2017 •

edited

Loading

frederikprijck Mar 24, 2017

gkalpak Mar 24, 2017

frederikprijck Mar 28, 2017

gkalpak Mar 28, 2017 •

edited

Loading

frederikprijck Mar 31, 2017 •

edited

Loading

gkalpak Apr 1, 2017

gkalpak Mar 24, 2017 •

edited

Loading

katsos commented Jun 20, 2017

frederikprijck commented Jun 20, 2017

Narretz commented Jun 29, 2017

gkalpak Jul 3, 2017

frederikprijck Jul 3, 2017 •

edited

Loading

gkalpak commented Jul 3, 2017

Narretz commented Jul 4, 2017

gkalpak commented Jul 4, 2017

evanjmg commented Jan 7, 2021

frederikprijck commented Jan 7, 2021

feature($http): allow differentiation between Request Error, Abort and Timeout #15847

feature($http): allow differentiation between Request Error, Abort and Timeout #15847

Conversation

frederikprijck commented Mar 22, 2017 • edited Loading

frederikprijck commented Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederikprijck Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederikprijck Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

frederikprijck Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

gkalpak Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

frederikprijck Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

katsos commented Jun 20, 2017

frederikprijck commented Jun 20, 2017

Narretz commented Jun 29, 2017

Choose a reason for hiding this comment

frederikprijck Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

gkalpak commented Jul 3, 2017

Narretz commented Jul 4, 2017

gkalpak commented Jul 4, 2017

evanjmg commented Jan 7, 2021

frederikprijck commented Jan 7, 2021

frederikprijck commented Mar 22, 2017 •

edited

Loading

frederikprijck commented Mar 22, 2017 •

edited

Loading

frederikprijck Mar 22, 2017 •

edited

Loading

frederikprijck Mar 22, 2017 •

edited

Loading

frederikprijck Mar 22, 2017 •

edited

Loading

gkalpak Mar 24, 2017 •

edited

Loading

gkalpak Mar 28, 2017 •

edited

Loading

frederikprijck Mar 31, 2017 •

edited

Loading

gkalpak Mar 24, 2017 •

edited

Loading

frederikprijck Jul 3, 2017 •

edited

Loading