fix(http): preserve config object when resolving from cache #9030
Conversation
| @@ -962,8 +962,7 @@ function $HttpProvider() { | |||
| if (isDefined(cachedResp)) { | |||
| if (isPromiseLike(cachedResp)) { | |||
| // cached request has already been sent, but there is no response yet | |||
| cachedResp.then(removePendingReq, removePendingReq); | |||
| return cachedResp; | |||
| cachedResp.then(resolvePromiseWithResult, resolvePromiseWithResult); | |||
There was a problem hiding this comment.
May it be that the Travis CI is failing because cachedResp is not returned any more here?
There was a problem hiding this comment.
travis is just flaky as usual, cachedResp shouldn't be returned here
There was a problem hiding this comment.
well, it's supposed to return the cached promise... now it's just returning a promise that will never be resolved, and won't even make a new request for it --- so operations on the returned promise are basically pointless.
There was a problem hiding this comment.
Also, how is this "preserving the config object", anyway?
There was a problem hiding this comment.
@caitp I think you are reading this wrong... We don't want to return the cached promise since this promise is resolved with the wrong config, this is the bug this PR fixes. Instead, we return a new promise that gets resolved with the correct result once the cached promise is resolved (this happens in resolvePromiseWithResult)
There was a problem hiding this comment.
exactly - the problem is that the cached promise returns the cached config which breaks a scenario where developers would add their "requestIds" in config that would never be resolved (the response would contain a previous request id).
unless there's some other way to push something with the request and have it in the callback.
petebacondarwin
force-pushed
the
master
branch
from
02dc2aa to
fd2d6c0
Compare
jeffbcross
force-pushed
the
master
branch
2 times, most recently
from
cad9560 to
f294244
Compare
|
rc3 released, no merge - will this go in rc4 or 1.3 stable? |
jeffbcross
force-pushed
the
master
branch
4 times, most recently
from
e8dc429 to
e83fab9
Compare
|
@petebacondarwin / @jeffbcross : as per comment in #9004, please consider a merge for this one in near future releases. |
|
@dragosrususv - I am afraid that AngularJS gets hundreds of issues submitted each week. It is very hard for the fairly small team at Google deal with them all at a rate that is acceptable for everyone. I guess that since you have gone into production with AngularJS, you have saved a considerable amount of development time over writing your application in vanilla JavaScript or building your own framework. Perhaps you could help us by commenting on and suggesting fixes for some of the many issues that need to be dealt with? This would lower the burden on the core team who would have more time to deal with your issue. The focus of the teams effort in the last couple of weeks was to ensure that any issues that required breaking changes were merged before we released 1.3.0 since we don't want to have breaking changes in 1.3.x. I am confident that your issue will be addressed in due course (most probably after ngEurope). Thanks in advance for your patience. |
|
@petebacondarwin : I apologize if my tone was too harsh. On my side I really waited for this as it was specifically marked and it was in our plan to upgrade, but unfortunately we are forced to go with a "manually edited" version of 1.3.0 (not a disaster, but a bit messy with notes on DIFFS and etc). It is true, AngularJS is saving time and money in terms of development (I've seen this in multiple projects). I'm advocating for customers and different companies to adhere to AngularJS - I truly believe it's a nice and efficient way to write frontend code. It can be a burden sometimes, but if you try to understand the inner flows, things look pretty logical and solutions often arise. I have tried a few times to open issues or items that we are facing; I still have a few that I didn't open, because the previous ones are either closed or delayed (#9246 or #9159 ). For this reason, although I had a ton of energy when I started this process, I always need a refill with patience each time I check the statuses (2-3 weeks and no activity at all). Thank you for taking time to reply here. |
petebacondarwin
force-pushed
the
master
branch
from
4dd5a20 to
998c61c
Compare
|
ping |
|
This one is waiting for a while now and it is a pretty trivial fix. PTAL |
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/. If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits. Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name. |
|
@shahata : changed your email by any chance? |
|
CLAs look good, thanks! |
|
/CC @petebacondarwin / @btford - could this small fix be included in one of the 1.3.x releases in near future? |
|
@dragosrususv adding to 1.3.6 milestone |
|
Hip hip hooray! :) |
|
There is almost an identical PR here: #6534 We should make sure that tests from both are passing if those are different - just to make sure that both cases are covered if not identical. |
|
@pkozlowski-opensource I just verified that the test from #6534 is passing in this PR. I'm not committing it though since it just repeats a test I already have. |
|
Awesome, thnx @shahata ! |
|
@shahata this one LGTM. I've left some comments regarding tests - nothing critical though. Let me know if you feel like changing any of the tests based on my remarks - if not it is good to go as-is. |
|
@pkozlowski-opensource I've added the test you requested regarding the first request failing. Truth is that it is a bit confusing, since Anyway, let me know if you want me to land this one... |
|
@shahata thnx for the update. I'm landing this PR in the current form. What you are saying about error cases is completely true and I can see how this is inconsistent. So what you are saying that depending on the timing the second request will fail or not. But the scenario would have to be:
Yeh, this is inconsistent, we might want to open a separate issue for this (or wait for a one from users). |
|
@shahata : so finally will this get fixed? are we able to trigger ajax requests and don't have these cached config problem? |
|
|
|
@pkozlowski-opensource : I see the close, I don't see any merge or REF to another PR that might fix this. This is a real issue in angularjs projects (I know 2 that need this). So finally we have to go on with our custom patches to angularjs? FYI: The problem here was different then the one described in last comments, as per description in #9004 |
|
@dragosrususv if you read through https://help.github.com/articles/closing-issues-via-commit-messages/ you will see than one can close a pull request / issue from a commit by sticking some keywords into this commit. This is what has happened with facfec9 which commit message says: So it means that a commit from this PR was merged into master. If you go to the mentioned commit you will see that this is part of the master branch so yes, it was merged. Furthermore inspecting changelog (https://github.com/angular/angular.js/blob/master/CHANGELOG.md#136-robofunky-danceblaster-2014-12-08) you will see that this commit was released as part of 1.3.6 Did you test your application with 1.3.6? If it works for you than great if not please open a separate issue with a clear description and a minimal live reproduce scenario using plunker or similar. This will help avoiding any confusion. But I was under the impression that you were after this particular fix as you were reviewing / commenting in this PR. |
|
Ouh, I apologize. I had like +4 tickets that got closed without any merge and @shahata has been helping me with PRs and I thought this was closed without merge again. |
|
Late, but I confirm this works! Thank you kindly! |
Closes #9004