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

fix($httpBackend): fixup patch for #9979 #9993

Closed
wants to merge 2 commits into from
Closed

fix($httpBackend): fixup patch for #9979 #9993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c480462
fix($httpBackend): compare timeoutId with undefined in completeRequest
gyx1000 Nov 10, 2014
78b026c
fix($httpBackend): fixup patch for #9979
caitp Nov 10, 2014
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
4 changes: 3 additions & 1 deletion src/ng/httpBackend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,9 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc

function completeRequest(callback, status, response, headersString, statusText) {
// cancel timeout and subsequent timeout promise resolution
timeoutId && $browserDefer.cancel(timeoutId);
if (timeoutId !== void 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Why the void 0 instead of undefined ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

smaller, can't be rebound accidentally

Copy link
Member

Choose a reason for hiding this comment

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

Considering undefined is used throughout the code (in fact I'be never seen any void 0 occurrence in the core), this just introduces inconsistency and leaves maintainers wonder what's special about this case.
I am very surprised that this suggestion came from a maintainability advocate like you 😃

We should either change all occurrences to void 0 or none.
And considering that there are already meassures taken against rebinding undefined, I think using undefined is pretty safe.

Copy link
Member

Choose a reason for hiding this comment

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

I tend to agree with @gkalpak - void 0 looks more "magical" and it is not the pattern we are using in the other parts of the code base. I would stick to if (timeoutId !== undefined) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would rather we change other cases of undefined to void 0 in the tree instead. void 0 is by definition an undefined value, but it's not a binding identifier, so accidents don't happen with it. That, and it's smaller. It's not something worth worrying about though, tbh

Copy link
Member

Choose a reason for hiding this comment

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

@caitp I hear what you say but maybe let's focus on landing this PR open a separate issue / PR if care about reworking all undefined to void 0.

What do you think about my comment regarding tests? Do you want to land yours that is similar to the existing one or just change the counter to start from 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think ideally, the counter should just restart for each test case, it doesn't make a lot of sense to have it continue across tests, and it doesn't seem to be used very much in the tree. But I'd rather not worry about it for this CL

Copy link
Member

Choose a reason for hiding this comment

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

OK, so should we change void 0 => undefined and land this one with the test you've created? I'm still a bit worried about its name / location in the file....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so should we change void 0 => undefined

I don't see why.

I'm still a bit worried about its name / location in the file....

I'm not, but if it makes you feel better it could be rearranged while landing.

$browserDefer.cancel(timeoutId);
}
jsonpDone = xhr = null;

callback(status, response, headersString, statusText);
Expand Down
13 changes: 13 additions & 0 deletions test/ng/httpBackendSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,19 @@ describe('$httpBackend', function() {
});


it('should cancel timeout on completeRequest()', function() {
fakeTimeout.ids.length = fakeTimeout.fns.length = fakeTimeout.delays.length = 0;
fakeTimeoutId = 0;
spyOn(fakeTimeout, 'cancel').andCallThrough();
$backend('GET', '/some-url', null, callback, void 0, 100);
xhr = MockXhr.$$lastInstance;
expect(fakeTimeout.cancel).not.toHaveBeenCalled();
xhr.status = 200;
xhr.onload();
expect(fakeTimeout.cancel).toHaveBeenCalledWith(1);
});


it('should normalize IE\'s 1223 status code into 204', function() {
callback.andCallFake(function(status) {
expect(status).toBe(204);
Expand Down