feat(http): allow caching for JSONP requests

[shahata]

Closes #1947

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#8356)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[shahata]

This was far more trivial than I initially expected 👀

[trirach1]

Hi
Le 26 juil. 2014 20:32, "Shahar Talmi" [email protected] a écrit :

Closes #1947 #1947

You can merge this Pull Request by running

git pull https://github.com/shahata/angular.js jsonp-cache

Or view, comment on, or merge it at:

#8356
Commit Summary

• feat(http): allow caching for JSONP requests

File Changes

• M src/ng/http.js
  https://github.com/angular/angular.js/pull/8356/files#diff-0 (3)
• M test/ng/httpSpec.js
  https://github.com/angular/angular.js/pull/8356/files#diff-1 (12)

Patch Links:

• https://github.com/angular/angular.js/pull/8356.patch
• https://github.com/angular/angular.js/pull/8356.diff

—
Reply to this email directly or view it on GitHub
#8356.

[petebacondarwin]

Hi @shahata - did you check that this actually works in practice, since the original issue discusses the fact that the JSON_CALLBACK label will be replaced with a new value every time so breaking the cache. I wonder if the ngMock $httpBackend is intercepting too early before this occurs?

[petebacondarwin]

Actually looking deeper into the code it seems that the cache check is done in $http and the JSON_CALLBACK replacement is done in $httpBackend so I guess we are OK with hitting the cache.

[shahata]

Yes, I also noticed that comment, but from what I see it is either wrong or maybe the code has changed since then, because now the url manipulation is decoupled nicely from the cache.

[shahata]

Here's the plunker I played with to see that it works in practice - http://plnkr.co/edit/XRCFYD7KlgvLTLGPtTcn?p=preview

[petebacondarwin]

Here is another version of the Plunker: http://plnkr.co/edit/bs7joENeXtEW6LzzvVBB?p=preview
Your one failed because I didn't have a valid twitter API key or something. On can see that the promise is requested and a promise is resolved repeatedly in the console but in the network tab you can see that only one real HTTP request is made.

[petebacondarwin]

So LGTM - I'll merge

[caitp]

JSONP requests are always essentially GET requests, so it would be cool if we could just rename the method to GET --- but this would take a bit more refactoring, so this is fine for now.

I think this implementation is generally fine, so LGTM