Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is there an equivalent to SDWebImageRefreshCached option? #35

Closed
rainypixels opened this issue Apr 24, 2015 · 7 comments
Closed

Is there an equivalent to SDWebImageRefreshCached option? #35

rainypixels opened this issue Apr 24, 2015 · 7 comments

Comments

@rainypixels
Copy link

Does Kingfisher have a KingfisherOptionsInfo option (or a plan to add one) that's an equivalent to SDWebImage's SDWebImageRefreshedCached option?

At first glance I thought ForceRefresh was the answer. But looking at your docs, that doesn't seem to be the case. It seems like ForceRefresh always loads the image from the Web while SDWebImage's SDWebImageRefreshedCached instead checks the HTTP caching control headers and honors them, i.e. the image is loaded from the Web when, say, the server doesn't return a 304-Not-Modified, otherwise it's pulled from the cache. The scenario this enables (new image, same url) is pretty common there days; my upcoming app relies on it as well.

Thanks for this library, btw. :) Very timely and much needed since there's really nothing that's pure Swift-based yet.

@onevcat
Copy link
Owner

onevcat commented Apr 25, 2015

Hi, @rainypixels

Yes, Kingfisher's ForceRefresh will always download the image from web again. I agree it make sense to check 304 status code and will try to see what I can do with that.

Thanks!

@onevcat
Copy link
Owner

onevcat commented May 1, 2015

@rainypixels Fixed in #42 . I decided to leave the handling detail to framework users and this wiki explained how to implement it with Kingfisher. Please let me know if it works for you or not.

Thanks!

@rainypixels
Copy link
Author

@onevcat Apologies! I've been heads down working on my code, so I just saw this.

First off, thanks for looking into this feature request so quickly! Much appreciated.

As I understand it (from your latest check-in and the wiki), you've modified Kingfisher to support downloading and caching, but its up to the framework users to manage caching/storing eTags, and passing them along to the server whenever necessary. Your wiki is very clear, and the solution you're proposing is elegant. Let me address your question:

Please let me know if it works for you or not.

I have a few concerns:

  1. I know you used NSUserDefaults for illustrative purposes. As you can imagine, in production users will have to roll their own NSCache implementation (or something similar) to store eTags. This is not a huge amount of overhead, but it does add some complexity. Some users will be OK with that, but my hunch is that the kind of user who would adopt your library is looking for a turnkey image download and caching solution, and this may be a bit of a deal-breaker for that user.
  2. I'm worried that the user implementation to intercept requests and responses is going to be prone to breaking over time as you update your framework because of the number of points of contact. Effectively, anyone who wants to support 304-Not-Modified responses is modifying core functionality of Kingfisher, e.g. modifying all requests through KingfisherManager.sharedManager.downloader.requestModifier. I think you anticipate this risk as well because your wiki starts with: This wiki page is wrote on 30 Apr, 2015, for Kingfisher 1.3.0. I hope I can remember to update it if something changes later. :-)
  3. Finally, I noticed that your implementation to support 304's classifies NotModified as an error. The 3xx class of status codes are redirection codes (only 4xx and 5xx are errors). From a framework perspective, I would be far more comfortable seeing a 304 reflected as what it is: a redirection code. This may seem like a tiny implementation detail, but I think it informs the architecture of the framework and its future extensibility (e.g. when other use cases come along in the 3xx class). In the near term, we can already see the effect it has had on the current implementation to handle 304's.

Finally: will this work for me? Honestly, I'm not entirely sure yet. I'm very careful about the dependencies I take, and the above issues are legitimate concerns for me. For instance, I am definitely in the group seeking a turnkey solution that doesn't even have to worry about managing a cache. But, again, it's too early for me to tell. Fortunately, I'm probably 6-8 weeks from having to worry about an image download/caching solution, so I don't have to make the decision now.

All that said, I truly appreciate your response and the effort (both for the check-in and the wiki)! I'll continue watching this space. :-)

@onevcat
Copy link
Owner

onevcat commented May 2, 2015

Hi, @rainypixels

Thanks for the reply.

  1. As I know, there are several solutions for a server to response 304. Instead of sending and checking ETag, there is also date checking on "Last-Modified". Since users may adopt to different strategies when they handle 304, so I decide to leave the detail implementation to the users. If it turns an ETag based system is a common request, it is also possible to extend the downloader class by adding an option to give a default implementation for ETag based 304 checking and downloading.
  2. With the consideration to flexibility, it is necessary to expose the request to framework users. In fact the request of Kingfisher is nothing more than a simple plain GET request for the URL. The core idea when implementing Kingfisher is keeping it simple for simple use case, and make it flexible and extendable whenever you need. And for the requestModifier, you should use it to modify these urls/requests which are necessary to do so, instead of all requests.
  3. Surely. Instead of an error, it should be a state. Although 304 code is now only used internally, it should be reconsidered.

@rainypixels
Copy link
Author

Makes sense on all fronts. I shall continue noodling on this in the meantime. I may have the need to take a generic caching solution dependency in any case, so I might just end up rolling with Kingfisher because I solidly support this sentiment:

The core idea when implementing Kingfisher is keeping it simple for simple use case

Maybe I'll even end up forking and sending you a pull request for a turnkey solution. I should know where I stand in time.

Thanks, again!

@onevcat
Copy link
Owner

onevcat commented May 4, 2015

Thanks for your kind words and effort.

@onevcat onevcat closed this as completed May 4, 2015
@BhavinBhadani
Copy link

this fromMemoryCacheOrRefresh not work for me .. still give me first cache image even if its changed

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

No branches or pull requests

3 participants