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

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

Closed
wants to merge 6 commits into from
Closed

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

wants to merge 6 commits into from

Conversation

tipycalFlow
Copy link

@tipycalFlow tipycalFlow commented Jan 19, 2017 •
edited
Loading

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Adding param xhrStatus to response and separate handlers for onerror, onabort and ontimeout

What is the current behavior? (You can also link to an open issue here)
Same handler for all error events

What is the new behavior (if this is a feature change)?
Separate handlers

Does this PR introduce a breaking change?
Probably not

Please check if the PR fulfills these requirements

Other information:
Some additional tests might be required in httpSpec.js and the docs would need updating as well.

@tipycalFlow
Adding param xhrStatus to response
89b6ca9
@tipycalFlow
Adding separate handlers for xhr
377a742 
Adding param xhrStatus to response and separate handlers for onerror, onabort, ontimeout
@tipycalFlow
Updating for xhrStatus changes
6e3628a
@tipycalFlow tipycalFlow mentioned this pull request Jan 19, 2017
Closed
@Narretz
Copy link
Contributor

Narretz commented Jan 20, 2017

Can you explain why you propose this change?

@Narretz Narretz added this to the Purgatory milestone Jan 20, 2017
@tipycalFlow
Copy link
Author

tipycalFlow commented Jan 21, 2017 •
edited
Loading

This patch is in reference to issue #15380 regarding xhr requests being force timed out after 60 seconds on iOS 10. The issue is that in src/ng/httpBackend.js, all error events have the same handlers:

xhr.onerror = requestError;
xhr.onabort = requestError;
xhr.ontimeout = requestError;

which makes it difficult to differentiate between these possibilities in the callback. So separate handlers will help developers better handle these different errors by being able to show the respective error messages, etc.

@Narretz Narretz changed the title Patch 1 $http: allow differentiation between Request Error, Abort and Timeout Jan 25, 2017
@Narretz
Copy link
Contributor

Narretz commented Jan 25, 2017

Sounds reasonable. I might actually need this myself in an app :)
Question: Is there a standard for the strings "Request Timed Out" , "Request Error", "Request Aborted"? Or did you take the most simple wording?

If we merge this, we also need to update the docs about this.

@Narretz Narretz modified the milestones: 1.6.x, Purgatory Jan 25, 2017
gkalpak
gkalpak suggested changes Jan 25, 2017
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need:

  1. More tests for the new feature (e.g. test for all possible values).
  2. Docs (as @Narretz mentioned already).
  3. Similar changes in the mocked $httpBackend in angular-mocks.

src/ng/http.js
} else {
resolvePromise(cachedResp, 200, {}, 'OK');
resolvePromise(cachedResp, 200, {}, 'OK', 'Request Completed');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this strings just represent values (like an enum), not user messages, I think we should keep them as concise as possible. I propose:

  • success
  • error
  • timeout
  • abort

test/ng/httpBackendSpec.js
@@ -90,7 +90,7 @@ describe('$httpBackend', function() {
});

it('should call completion function with xhr.statusText if present', function() {
callback.and.callFake(function(status, response, headers, statusText) {
callback.and.callFake(function(status, response, headers, statusText, xhrStatus) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is redundant.

test/ng/httpBackendSpec.js
@@ -102,7 +102,7 @@ describe('$httpBackend', function() {
});

it('should call completion function with empty string if not present', function() {
callback.and.callFake(function(status, response, headers, statusText) {
callback.and.callFake(function(status, response, headers, statusText, xhrStatus) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is redundant.

Narretz pushed a commit to Narretz/angular.js that referenced this pull request Jan 26, 2017
@tipycalFlow @Narretz
Updating for xhrStatus changes
174f6b0 
Closes angular#15626
@tipycalFlow
Copy link
Author

tipycalFlow commented Feb 7, 2017 •
edited
Loading

@gkalpak can you please add more tests and documentation as required?

@frederikprijck
Copy link
Contributor

I'll take a look to add more tests cases and update the docs.

@Narretz
Copy link
Contributor

Narretz commented Mar 6, 2017

@frederikprijck Much appreciated!

@frederikprijck frederikprijck mentioned this pull request Mar 22, 2017
Closed
@Narretz
Copy link
Contributor

Narretz commented Apr 20, 2017

Continued in #15847

@Narretz Narretz closed this Apr 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gkalpak gkalpak gkalpak requested changes

Assignees
No one assigned
Labels
cla: yes component: $http needs: review needs: work type: feature
Projects
None yet
Milestone
1.6.x
Development

Successfully merging this pull request may close these issues.
5 participants
@tipycalFlow @Narretz @frederikprijck @gkalpak @googlebot