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

docs($http): expand caching section to cover FAQ #13003

Closed
wants to merge 2 commits into from
Closed

docs($http): expand caching section to cover FAQ #13003

wants to merge 2 commits into from

Conversation

ryanhart2
Copy link
Contributor

  • add note that only GET & JSONP requests are cached
  • add note that cache keys are the url and search params (headers not considered)
  • add note that cache-control header on response does not affect angular caching
  • add note that $httpProvider.defaults.cache can be used to set cache default (in addition to $http.defaults.cache)
  • expand note on how default cache property affects requests with cache set to true

Fixes #11101

*
* * Only GET and JSONP requests are cached.
* * The cache key is the request url including search parameters; headers are not considered.
* * Cached responses are return asynchronously, in the same way as responses from the server.
Copy link
Member

Choose a reason for hiding this comment

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

return --> returned

@ryanhart2
Copy link
Contributor Author

@gkalpak, I fixed the spelling error and also uppercased 'URL' to be consistent with the rest of the comments. I also added Fixes #11101 to the commit comment.

*
* If you set the default cache to `false` then only requests that specify their own custom
* cache object will be cached.
* The default cache can also be set to `false`, which will mean that only requests that specify
Copy link
Member

Choose a reason for hiding this comment

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

This sentence seems incorrect (from taking a quick look at the code).

@ryanhart2
Copy link
Contributor Author

@gkalpak, you are correct: a default cache value of false can be overridden by a cache config value of true as well as a custom cache object, so I will update.

This means that rows 260-263 of the file (describing the defaults object) are also incorrect, so I will include a revision of these in my commit as well.

There is also no mention of the effect of setting the default cache to true (it results in the $http cache being used when the config cache is undefined), so I will expand my comments to include this.

I set up this plunk to test the various combinations.

@Narretz
Copy link
Contributor

Narretz commented Jan 19, 2016

@ryanhart2 Did you ever update this PR?

@Narretz Narretz added this to the Backlog milestone Jan 19, 2016
@ryanhart2
Copy link
Contributor Author

@Narretz Apologies, I didn't and had forgotten about it. I will have a look this weekend and try to get it done.

@Narretz
Copy link
Contributor

Narretz commented Jan 21, 2016

Am 21.01.2016 um 15:04 schrieb Ryan Hart:

@Narretz https://github.com/Narretz Apologies, I didn't and had
forgotten about it. I will have a look this weekend and try to get it
done.


Reply to this email directly or view it on GitHub
#13003 (comment).

No problem. I just wouldn't want to see good work go to waste!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@Narretz
Copy link
Contributor

Narretz commented Feb 11, 2016

@ryanhart2 why did you close it?

@ryanhart2 ryanhart2 reopened this Feb 11, 2016
@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

@ryanhart2
Copy link
Contributor Author

@Narretz Just struggling to do the rebase... I think I am getting there!

@ryanhart2
Copy link
Contributor Author

@Narretz , @gkalpak I have updated this pull request to more accurately describe how http caching works. Appreciate if you have time to review and provide feedback. Thanks.

* GET request, otherwise if a cache instance built with
* {@link ng.$cacheFactory $cacheFactory}, this cache will be used for
* caching.
* - **cache** – `{boolean|Object}` – A value to enable caching of the HTTP response. See
Copy link
Member

Choose a reason for hiding this comment

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

IMO it is still useful to mention that the value must be either boolean or a Cache object created with $cacheFactory.

@gkalpak
Copy link
Member

gkalpak commented Feb 17, 2016

I left a couple of comments (actually it's the same comment twice 😛 ).
BTW, @ryanhart2, you don't need to rebase. (In fact it's preferrable to just push a new commit with the changes 😃)

ryanhart2 added 2 commits February 18, 2016 05:00
add note that cache keys are the url and search params (headers not considered)
add note that cache-control header on response does not affect angular caching
add note that $httpProvider.defaults.cache can be used to set cache default (in addition to $http.defaults.cache)
revise explanation of how caching can be enabled using cache.config value and cache default value
Fixes #11101
…lue must be either boolean or a Cache object created with $cacheFactory
@ryanhart2
Copy link
Contributor Author

Thanks @gkalpak. I agree, the revised version now reads better. Thanks for the hint on the rebase. I have pushed a new commit addressing your suggestions. Note that, on the same lines, I also updated the words "enable caching" to "enable or disable caching", given that the values could be false.

@gkalpak gkalpak closed this in 84c04b0 Feb 17, 2016
gkalpak pushed a commit that referenced this pull request Feb 17, 2016
Included changes:

* Point out that only GET & JSONP requests are cached.
* Explain that the URL+search params are used as cache keys (headers not considered).
* Add note about cache-control headers on response not affecting Angular caching.
* Mention `$httpProvider.defaults.cache` (in addition to `$http.defaults.cache`).
* Clear up how `defaults.cache` and `config.cache` are taken into account for determining the
  caching behavior for each request.

Fixes #11101
Closes #13003
gkalpak pushed a commit that referenced this pull request Feb 17, 2016
Included changes:

* Point out that only GET & JSONP requests are cached.
* Explain that the URL+search params are used as cache keys (headers not considered).
* Add note about cache-control headers on response not affecting Angular caching.
* Mention `$httpProvider.defaults.cache` (in addition to `$http.defaults.cache`).
* Clear up how `defaults.cache` and `config.cache` are taken into account for determining the
  caching behavior for each request.

Fixes #11101
Closes #13003
@gkalpak
Copy link
Member

gkalpak commented Feb 17, 2016

Backported to v1.5.x (2a7d4c1) and v1.4.x (b9d3625).
Thx @ryanhart2 for working on this !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants