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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
89b6ca9
Adding param xhrStatus to response
tipycalFlow Jan 19, 2017
377a742
Adding separate handlers for xhr
tipycalFlow Jan 19, 2017
6e3628a
Updating for xhrStatus changes
tipycalFlow Jan 19, 2017
2806943
Concise xhr status
tipycalFlow Jan 27, 2017
c5e0a31
Concise xhr status
tipycalFlow Jan 27, 2017
7d13e73
Concise xhr status
tipycalFlow Jan 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions src/ng/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1268,9 +1268,9 @@ function $HttpProvider() {
} else {
// serving from cache
if (isArray(cachedResp)) {
resolvePromise(cachedResp[1], cachedResp[0], shallowCopy(cachedResp[2]), cachedResp[3]);
resolvePromise(cachedResp[1], cachedResp[0], shallowCopy(cachedResp[2]), cachedResp[3], cachedResp[4]);
} 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

}
}
} else {
Expand Down Expand Up @@ -1327,18 +1327,18 @@ function $HttpProvider() {
* - resolves the raw $http promise
* - calls $apply
*/
function done(status, response, headersString, statusText) {
function done(status, response, headersString, statusText, xhrStatus) {
if (cache) {
if (isSuccess(status)) {
cache.put(url, [status, response, parseHeaders(headersString), statusText]);
cache.put(url, [status, response, parseHeaders(headersString), statusText, xhrStatus]);
} else {
// remove promise from the cache
cache.remove(url);
}
}

function resolveHttpPromise() {
resolvePromise(response, status, headersString, statusText);
resolvePromise(response, status, headersString, statusText, xhrStatus);
}

if (useApplyAsync) {
Expand All @@ -1353,7 +1353,7 @@ function $HttpProvider() {
/**
* Resolves the raw $http promise.
*/
function resolvePromise(response, status, headers, statusText) {
function resolvePromise(response, status, headers, statusText, xhrStatus) {
//status: HTTP response status code, 0, -1 (aborted by timeout / promise)
status = status >= -1 ? status : 0;

Expand All @@ -1362,12 +1362,13 @@ function $HttpProvider() {
status: status,
headers: headersGetter(headers),
config: config,
statusText: statusText
statusText: statusText,
xhrStatus: xhrStatus
});
}

function resolvePromiseWithResult(result) {
resolvePromise(result.data, result.status, shallowCopy(result.headers()), result.statusText);
resolvePromise(result.data, result.status, shallowCopy(result.headers()), result.statusText, result.xhrStatus);
}

function removePendingReq() {
Expand Down
23 changes: 16 additions & 7 deletions src/ng/httpBackend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
var jsonpDone = jsonpReq(url, callbackPath, function(status, text) {
// jsonpReq only ever sets status to 200 (OK), 404 (ERROR) or -1 (WAITING)
var response = (status === 200) && callbacks.getResponse(callbackPath);
completeRequest(callback, status, response, '', text);
completeRequest(callback, status, response, '', text, '');
callbacks.removeCallback(callbackPath);
});
} else {
Expand Down Expand Up @@ -99,18 +99,27 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
status,
response,
xhr.getAllResponseHeaders(),
statusText);
statusText,
'Request Completed');
};

var requestError = function() {
// The response is always empty
// See https://xhr.spec.whatwg.org/#request-error-steps and https://fetch.spec.whatwg.org/#concept-network-error
completeRequest(callback, -1, null, null, '');
completeRequest(callback, -1, null, null, '', 'Request Error');
};

var requestAborted = function() {
completeRequest(callback, -1, null, null, '', 'Request Aborted');
};

var requestTimedOut = function() {
completeRequest(callback, -1, null, null, '', 'Request Timed Out');
};

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

forEach(eventHandlers, function(value, key) {
xhr.addEventListener(key, value);
Expand Down Expand Up @@ -160,14 +169,14 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
}
}

function completeRequest(callback, status, response, headersString, statusText) {
function completeRequest(callback, status, response, headersString, statusText, xhrStatus) {
// cancel timeout and subsequent timeout promise resolution
if (isDefined(timeoutId)) {
$browserDefer.cancel(timeoutId);
}
jsonpDone = xhr = null;

callback(status, response, headersString, statusText);
callback(status, response, headersString, statusText, xhrStatus);
}
};

Expand Down
11 changes: 6 additions & 5 deletions test/ng/httpBackendSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

expect(statusText).toBe('OK');
});

Expand All @@ -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.

expect(statusText).toBe('');
});

Expand Down Expand Up @@ -155,11 +155,12 @@ describe('$httpBackend', function() {
});

it('should not try to read response data when request is aborted', function() {
callback.and.callFake(function(status, response, headers, statusText) {
callback.and.callFake(function(status, response, headers, statusText, xhrStatus) {
expect(status).toBe(-1);
expect(response).toBe(null);
expect(headers).toBe(null);
expect(statusText).toBe('');
expect(xhrStatus).toBe('Request Aborted');
});
$backend('GET', '/url', null, callback, {}, 2000);
xhr = MockXhr.$$lastInstance;
Expand All @@ -174,11 +175,12 @@ describe('$httpBackend', function() {
});

it('should complete the request on timeout', function() {
callback.and.callFake(function(status, response, headers, statusText) {
callback.and.callFake(function(status, response, headers, statusText, xhrStatus) {
expect(status).toBe(-1);
expect(response).toBe(null);
expect(headers).toBe(null);
expect(statusText).toBe('');
expect(xhrStatus).toBe('Request Timed Out');
});
$backend('GET', '/url', null, callback, {});
xhr = MockXhr.$$lastInstance;
Expand Down Expand Up @@ -511,4 +513,3 @@ describe('$httpBackend', function() {
});
});
});