fix($httpBackend): fixup patch for #9979 #9993
Conversation
httpBackend with ngMock browser.defer can never cancel the first deferredFn because the timeoutId returned by defer for the first fn is a zero value. Compare timeoutId with undefined fix this issue.
|
I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS. Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match. If you signed the CLA as a corporation, please let us know the company's name. Thanks a bunch! PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR. |
| @@ -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) { | |||
There was a problem hiding this comment.
Why the void 0 instead of undefined ?
There was a problem hiding this comment.
smaller, can't be rebound accidentally
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) {
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
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.
|
@caitp I've left some minor comments. Looking at the tests, though, I've realised that we are not using mocked browser.deffer (BTW, there is even a TODO from Vojta for this..) which makes the test look funny... As noted your test looks very similar to the one that already exists and changing this line from So it is up to you to decide if we want to have a separate test or just start fake timeout ids counter from 0. |
|
Right, the TODO from Vojta will not work if this issue is not fixed. (I discovered it when I try to complete this todo).This test angular.js/test/ng/httpBackendSpec.js Line 221 in ebc3b7b |
|
@caitp I was about to merge this one but then I've realised that there is something wrong with a test - it seems to be passing even without a fix. It might be that I've messed up something with my env so could you please verify that your test breaks if the fix is not present? On a separate note we should really do something about the way timeouts are mocked in the httpBackendSpec - it is rather messy atm... |
|
@pkozlowski-opensource in this comment #9979 (comment) I asked you to use the defer mock instead of write a new test because the fix will be tested by angular.js/test/ng/httpBackendSpec.js Line 221 in ebc3b7b |
|
@gyx1000 definitivelly - if we are able to replace fakeTimeout with defer mock - this would be even better! Could you please open a new pull request with a fix and changes to the tests? |
|
We've got a version that gets rid of shaky tests: #10177 - let's focus on this one. |
Builds ontop of c480462 from #9979