From eaf0cd726b9a6a2f64e85bdc8f7703f1ded6ac5c Mon Sep 17 00:00:00 2001 From: Michal Raczkowski Date: Fri, 12 Feb 2016 18:30:36 +0100 Subject: [PATCH 1/3] feat($http,$resource): statusText resolved in timeout promise Timeout-promise resolved-value will be passed as response error statusText ```js $http({ method: 'GET', url: '...', timeout: $timeout(function() { return "something"; }, 500) }).catch(function(err) { console.log(err.statusText); // something }); ``` This feature will also help distinguish cancelled $resource requests from timed-out $resource requests. Before both cases resulted with the same error object: ```{status: -1, statusText: '', ...}``` and were not giving much options to handle them in different way. Now cancelled requests will come with statusText='cancelled' and timed-out requests with statusText='timeout' in error object. --- src/ng/httpBackend.js | 9 ++++++--- src/ngMock/angular-mocks.js | 8 ++++++-- src/ngResource/resource.js | 11 +++++++++-- test/ng/httpBackendSpec.js | 25 ++++++++++++++++++++++++- test/ngMock/angular-mocksSpec.js | 18 ++++++++++++++++++ test/ngResource/resourceSpec.js | 26 +++++++++++++++++++++++--- 6 files changed, 86 insertions(+), 11 deletions(-) diff --git a/src/ng/httpBackend.js b/src/ng/httpBackend.js index 0b16b34a7748..ada615e019d1 100644 --- a/src/ng/httpBackend.js +++ b/src/ng/httpBackend.js @@ -105,10 +105,12 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc statusText); }; + var timeoutStatusText; + 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, timeoutStatusText || ''); }; xhr.onerror = requestError; @@ -145,7 +147,8 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc } - function timeoutRequest() { + function timeoutRequest(statusText) { + timeoutStatusText = statusText; jsonpDone && jsonpDone(); xhr && xhr.abort(); } @@ -155,7 +158,7 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc if (isDefined(timeoutId)) { $browserDefer.cancel(timeoutId); } - jsonpDone = xhr = null; + jsonpDone = xhr = timeoutStatusText = null; callback(status, response, headersString, statusText); $browser.$$completeOutstandingRequest(noop); diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js index 3a843e2bc7ac..86496a96a734 100644 --- a/src/ngMock/angular-mocks.js +++ b/src/ngMock/angular-mocks.js @@ -1346,11 +1346,15 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) { copy(response[3] || '')); } - function handleTimeout() { + function handleTimeout(statusText) { for (var i = 0, ii = responses.length; i < ii; i++) { if (responses[i] === handleResponse) { responses.splice(i, 1); - callback(-1, undefined, ''); + if (statusText) { + callback(-1, undefined, '', statusText); + } else { + callback(-1, undefined, ''); + } break; } } diff --git a/src/ngResource/resource.js b/src/ngResource/resource.js index e58b0aecac40..380813ace1a8 100644 --- a/src/ngResource/resource.js +++ b/src/ngResource/resource.js @@ -658,7 +658,9 @@ angular.module('ngResource', ['ng']). httpConfig.timeout = timeoutDeferred.promise; if (numericTimeout) { - numericTimeoutPromise = $timeout(timeoutDeferred.resolve, numericTimeout); + numericTimeoutPromise = $timeout(function() { + timeoutDeferred.resolve("timeout"); + }, numericTimeout); } } @@ -729,7 +731,12 @@ angular.module('ngResource', ['ng']). // - return the instance / collection value.$promise = promise; value.$resolved = false; - if (cancellable) value.$cancelRequest = timeoutDeferred.resolve; + + if (cancellable) { + value.$cancelRequest = function() { + timeoutDeferred.resolve("cancelled"); + }; + } return value; } diff --git a/test/ng/httpBackendSpec.js b/test/ng/httpBackendSpec.js index cd356959578d..8aeb621ca683 100644 --- a/test/ng/httpBackendSpec.js +++ b/test/ng/httpBackendSpec.js @@ -173,8 +173,9 @@ describe('$httpBackend', function() { it('should abort request on timeout promise resolution', inject(function($timeout) { - callback.andCallFake(function(status, response) { + callback.andCallFake(function(status, response, headers, statusText) { expect(status).toBe(-1); + expect(statusText).toBe(''); }); $backend('GET', '/url', null, callback, {}, $timeout(noop, 2000)); @@ -190,6 +191,28 @@ describe('$httpBackend', function() { })); + it('should abort request on timeout promise resolution with statusText', inject(function($timeout) { + callback.andCallFake(function(status, response, headers, statusText) { + expect(status).toBe(-1); + expect(statusText).toBe('whatever'); + }); + + $backend('GET', '/url', null, callback, {}, $timeout(function() { + return "whatever"; + }, 2000)); + + xhr = MockXhr.$$lastInstance; + spyOn(xhr, 'abort'); + + $timeout.flush(); + expect(xhr.abort).toHaveBeenCalledOnce(); + + xhr.status = 0; + xhr.onabort(); + expect(callback).toHaveBeenCalledOnce(); + })); + + it('should not abort resolved request on timeout promise resolution', inject(function($timeout) { callback.andCallFake(function(status, response) { expect(status).toBe(200); diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js index 281b0763abea..0ecb05353d05 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -1399,6 +1399,24 @@ describe('ngMock', function() { }); + it('should abort requests when timeout promise resolves with statusText', function() { + hb.expect('GET', '/url1').respond(200); + + var canceler, then = jasmine.createSpy('then').andCallFake(function(fn) { + canceler = fn; + }); + + hb('GET', '/url1', null, callback, null, {then: then}); + expect(typeof canceler).toBe('function'); + + canceler('whatever'); // simulate promise resolution + + expect(callback).toHaveBeenCalledWith(-1, undefined, '', 'whatever'); + hb.verifyNoOutstandingExpectation(); + hb.verifyNoOutstandingRequest(); + }); + + it('should abort requests when timeout passed as a numeric value', inject(function($timeout) { hb.expect('GET', '/url1').respond(200); diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index c032b2ad63a1..d3fba2dda580 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -1544,7 +1544,13 @@ describe('cancelling requests', function() { } }); - CreditCard.get(); + var creditCard = CreditCard.get(); + + creditCard.$promise.catch(function(err) { + expect(err.status).toEqual(-1); + expect(err.statusText).toEqual("timeout"); + }); + $timeout.flush(); expect($httpBackend.flush).toThrow(new Error('No pending request to flush !')); @@ -1563,7 +1569,14 @@ describe('cancelling requests', function() { } }); - CreditCard.get().$cancelRequest(); + var creditCard = CreditCard.get(); + + creditCard.$promise.catch(function(err) { + expect(err.status).toEqual(-1); + expect(err.statusText).toEqual("cancelled"); + }); + + creditCard.$cancelRequest(); expect($httpBackend.flush).toThrow(new Error('No pending request to flush !')); CreditCard.get(); @@ -1581,7 +1594,14 @@ describe('cancelling requests', function() { } }); - CreditCard.get().$cancelRequest(); + var creditCard = CreditCard.get(); + + creditCard.$promise.catch(function(err) { + expect(err.status).toEqual(-1); + expect(err.statusText).toEqual("cancelled"); + }); + + creditCard.$cancelRequest(); expect($httpBackend.flush).toThrow(new Error('No pending request to flush !')); CreditCard.get(); From ebbb85ca68e4cfd06775d5ec7c75dede0f3d8884 Mon Sep 17 00:00:00 2001 From: Michal Raczkowski Date: Tue, 16 Feb 2016 17:33:17 +0100 Subject: [PATCH 2/3] fixup: add spies in unit tests --- test/ngResource/resourceSpec.js | 36 +++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index d3fba2dda580..23948f566cfc 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -1544,19 +1544,23 @@ describe('cancelling requests', function() { } }); - var creditCard = CreditCard.get(); - - creditCard.$promise.catch(function(err) { + var errorCB = jasmine.createSpy('error').andCallFake(function(err) { expect(err.status).toEqual(-1); expect(err.statusText).toEqual("timeout"); }); + var creditCard = CreditCard.get(); + + creditCard.$promise.catch(errorCB); + $timeout.flush(); expect($httpBackend.flush).toThrow(new Error('No pending request to flush !')); + expect(errorCB).toHaveBeenCalledOnce(); + errorCB.reset(); CreditCard.get(); expect($httpBackend.flush).not.toThrow(); - + expect(errorCB).not.toHaveBeenCalled(); }); it('should cancel the request (if cancellable), when calling `$cancelRequest`', function() { @@ -1569,18 +1573,23 @@ describe('cancelling requests', function() { } }); - var creditCard = CreditCard.get(); - - creditCard.$promise.catch(function(err) { + var errorCB = jasmine.createSpy('error').andCallFake(function(err) { expect(err.status).toEqual(-1); expect(err.statusText).toEqual("cancelled"); }); + var creditCard = CreditCard.get(); + + creditCard.$promise.catch(errorCB); + creditCard.$cancelRequest(); expect($httpBackend.flush).toThrow(new Error('No pending request to flush !')); + expect(errorCB).toHaveBeenCalledOnce(); + errorCB.reset(); CreditCard.get(); expect($httpBackend.flush).not.toThrow(); + expect(errorCB).not.toHaveBeenCalled(); }); it('should cancel the request, when calling `$cancelRequest` in cancellable actions with timeout defined', function() { @@ -1594,18 +1603,23 @@ describe('cancelling requests', function() { } }); + var errorCB = jasmine.createSpy('error').andCallFake(function(err) { + expect(err.status).toEqual(-1); + expect(err.statusText).toEqual("cancelled"); + }); + var creditCard = CreditCard.get(); - creditCard.$promise.catch(function(err) { - expect(err.status).toEqual(-1); - expect(err.statusText).toEqual("cancelled"); - }); + creditCard.$promise.catch(errorCB); creditCard.$cancelRequest(); expect($httpBackend.flush).toThrow(new Error('No pending request to flush !')); + expect(errorCB).toHaveBeenCalledOnce(); + errorCB.reset(); CreditCard.get(); expect($httpBackend.flush).not.toThrow(); + expect(errorCB).not.toHaveBeenCalled(); }); it('should reset `$cancelRequest` after the response arrives', function() { From d8b9f298c856d2ee9478f1242e6a494bd4d79246 Mon Sep 17 00:00:00 2001 From: Michal Raczkowski Date: Wed, 17 Feb 2016 22:30:56 +0100 Subject: [PATCH 3/3] fixup: simplified callback call --- src/ngMock/angular-mocks.js | 6 +----- test/ngMock/angular-mocksSpec.js | 4 ++-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js index 86496a96a734..00ed5e2aa202 100644 --- a/src/ngMock/angular-mocks.js +++ b/src/ngMock/angular-mocks.js @@ -1350,11 +1350,7 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) { for (var i = 0, ii = responses.length; i < ii; i++) { if (responses[i] === handleResponse) { responses.splice(i, 1); - if (statusText) { - callback(-1, undefined, '', statusText); - } else { - callback(-1, undefined, ''); - } + callback(-1, undefined, '', statusText); break; } } diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js index 0ecb05353d05..b5e754e74131 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -1393,7 +1393,7 @@ describe('ngMock', function() { canceler(); // simulate promise resolution - expect(callback).toHaveBeenCalledWith(-1, undefined, ''); + expect(callback).toHaveBeenCalledWith(-1, undefined, '', undefined); hb.verifyNoOutstandingExpectation(); hb.verifyNoOutstandingRequest(); }); @@ -1423,7 +1423,7 @@ describe('ngMock', function() { hb('GET', '/url1', null, callback, null, 200); $timeout.flush(300); - expect(callback).toHaveBeenCalledWith(-1, undefined, ''); + expect(callback).toHaveBeenCalledWith(-1, undefined, '', undefined); hb.verifyNoOutstandingExpectation(); hb.verifyNoOutstandingRequest(); }));