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

feat($http,$resource): statusText resolved in timeout promise #14021

Closed
wants to merge 3 commits into from
Closed

feat($http,$resource): statusText resolved in timeout promise #14021

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eaf0cd7
feat($http,$resource): statusText resolved in timeout promise
mrac Feb 12, 2016
ebbb85c
fixup: add spies in unit tests
mrac Feb 16, 2016
d8b9f29
fixup: simplified callback call
mrac Feb 17, 2016
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
9 changes: 6 additions & 3 deletions src/ng/httpBackend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -145,7 +147,8 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
}


function timeoutRequest() {
function timeoutRequest(statusText) {
timeoutStatusText = statusText;
jsonpDone && jsonpDone();
xhr && xhr.abort();
}
Expand All @@ -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);
Expand Down
8 changes: 6 additions & 2 deletions src/ngMock/angular-mocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just always call callback(-1, undefined, '', statusText) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because some unit tests use spy's toHaveBeenCalledWith() method which is strict regarding the number of parameters, and I didn't want to break the old API. With undefined as the forth parameter those tests will fail, so may be some third-party modules would break if they test for it. It's minor however, and if you agree, we can just change it.

Copy link
Member

Choose a reason for hiding this comment

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

The advice in the docs is that "you should never need to use this service directly" and it's use as a function isn't even documented (even more so the arguments of the done callback).
I really think it's a non-issue, so go ahead and use the simpler version 😄

callback(-1, undefined, '', statusText);
} else {
callback(-1, undefined, '');
}
break;
}
}
Expand Down
11 changes: 9 additions & 2 deletions src/ngResource/resource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

This can be also written as:

numericTimeoutPromise = $timeout(timeoutDeferred.resolve, numericTimeout, false, 'timeout');
// OR
numericTimeoutPromise = $timeout(timeoutDeferred.resolve.bind(null, 'timeout'), numericTimeout);
// OR
numericTimeoutPromise = $timeout(angular.bind(timeoutDeferred, timeoutDeferred.resolve, 'timeout'), numericTimeout);

Copy link
Member

Choose a reason for hiding this comment

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

But...you know...it's better to stay as is 😥

}
}

Expand Down Expand Up @@ -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;
}
Expand Down
25 changes: 24 additions & 1 deletion test/ng/httpBackendSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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);
Expand Down
18 changes: 18 additions & 0 deletions test/ngMock/angular-mocksSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
26 changes: 23 additions & 3 deletions test/ngResource/resourceSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1544,7 +1544,13 @@ describe('cancelling requests', function() {
}
});

CreditCard.get();
var creditCard = CreditCard.get();

creditCard.$promise.catch(function(err) {
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to also verify that the function has been called (e.g. by using a spy).
(Here and in the tests below.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thnx. I pushed a fix

expect(err.status).toEqual(-1);
expect(err.statusText).toEqual("timeout");
});

$timeout.flush();
expect($httpBackend.flush).toThrow(new Error('No pending request to flush !'));

Expand All @@ -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();
Expand All @@ -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();
Expand Down