Allow creating a ASPINRemoteImageDownloader that ignores caches

[plarson]

Moving this change from facebookarchive/AsyncDisplayKit#3257

[CLAassistant]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ Adlai-Holler
✅ garrettmoon
❌ Phil Larson

---

Phil Larson seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[garrettmoon]

@plarson to continue the conversation here, would it be better to expose the sessionManager instead of adding this option?

[plarson]

@garrettmoon Exposing session manager would be helpful for adding custom HTTP headers.

For the caching issue, it seems to me that the best way is specifying PINRemoteImageManagerDownloadOptionsIgnoreCache?

It's checked in -[PINRemoteImageManager objectForURL:processorKey:key:options:completion:], which isn't related to the sessionManager. I see it as a race condition issue like checking if a file exists before opening it with NSFileManager instead of simply trying to open it and let it fail.

Clearing the cache for a URL before setting it on ASNetworkImageNode is technically a tiny performance hit since the app has to do the work to cache/clear an image that I am trying to always load from the network.

If the only way is removing the URL explicitly from the cache, then maybe ASImageCacheProtocol should be updated to have a method such as -(void)clearFetchedImageFromCacheWithURL:(NSURL *)URL includingDiskCache:(BOOL)includingDiskCache ?

[plarson]

In my opinion it might be options/flags added to ASNetworkImageNode that flows through ASImageDownloaderProtocol and -[ASImageDownloaderProtocol downloadImageWithURL:callbackQueue:downloadProgress:completion:] would change to accept an options argument that is analogous to PINRemoteImageManagerDownloadOptions. However, that would be a bigger change.

[ghost]

	1 Warning
⚠️	Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.

Generated by 🚫 Danger

[appleguy]

I'm in favor of providing more control to users of ASNetworkImageNode, in terms of caching behavior. So a config object / struct should be considered, although we might want to have it set at a class level rather than an instance level.

Among the parameters that would be useful to configure are properties on the PINDiskCache, like byteLimit. But also, I definitely agree we should make it easier to decouple from these frameworks and run without caching, etc.

[appleguy]

@garrettmoon any thoughts on the best way to support this?

[garrettmoon]

I feel like the underlying issue is that it's not straightforward to subclass ASPINRemoteImageManager. If we could make that easier we'd be able to support many more scenarios?

[appleguy]

@garrettmoon Yes, that could be helpful. I think there is probably a small number of knobs that should exist without subclassing, though; in particular, if there were only one, turning off the cache is likely to be the most common. I've seen a number of cases where PINCache is linked for other usages in an app, but a disk cache is not desired (or in fact allowed, due to it not handling server-specified expiry policies).

Any more specific guidance for the author of this PR? If you have thoughts on how we could make the subclassing option easy and clean and allow cache disabling as part of that, it would definitely be a step forward from today.

[Adlai-Holler]

Note: #747 provides a great place to specify your custom image manager.

[nguyenhuy]

Note: #1124 makes it easy to specify a custom, preconfiged image manager.

[nguyenhuy]

Based on the last 2 comments, I suppose we can close this old PR. Please feel free to reopen/open a new one if need to. Thank you all.