Create URLDataCache and DataDownloader #164
Conversation
| @discardableResult | ||
| public func downloadData(for request: URLRequest, completion: @escaping CompletionHandler) -> SessionTaskProxyType? { | ||
| var proxy: SessionTaskProxyType? | ||
| let completionQueue = self.completionQueue ?? .current ?? .main |
There was a problem hiding this comment.
So we could pass in a background queue to download data. But if left nil it'll default to current or main?
There was a problem hiding this comment.
That's how it reads to me. Do we want to put this in the init?
There was a problem hiding this comment.
We wouldn't want to put this in init.
We might want to init on a different thread than we run the downloader on. It seems more of a convenience if you know you're already going to be on the thread you want when calling downloadData
| strongSelf.sessionProxyMap[cacheIdentifier] = nil | ||
| strongSelf.completionHandlerMap[cacheIdentifier]?.forEach(execute) | ||
| strongSelf.completionHandlerMap[cacheIdentifier] = nil | ||
| } |
There was a problem hiding this comment.
Could you please explain the need for intentional retain cycles a bit more?
There was a problem hiding this comment.
@bconway99 If you look at the comment above that
// Strongly capture self within the completion handler to ensure
// DataDownloader is persisted long enough to respond
Since there are multiple asynchronous elements in this method we want to make sure we set the data we need before the objects are released
|
|
||
| waitForExpectations(timeout: 5) | ||
| } | ||
|
|
There was a problem hiding this comment.
Looks great, I'd just recommend adding the "GIVEN, WHEN, THEN" statements to the above tests as well.
| return _data(for: request) | ||
| } | ||
|
|
||
| private func _data(for request: URLRequest) -> NSData? { |
There was a problem hiding this comment.
Wondering if the private functions should be removed since they both are called from public ones without any extra logic. Is there some other reason for breaking them up?
|
|
||
| // Strongly capture self within the completion handler to ensure | ||
| // DataDownloader is persisted long enough to respond | ||
| let strongSelf = self |
There was a problem hiding this comment.
with the above guard statement (line 79) defined as it is self here is non-optional unless I'm misreading the scopes
There was a problem hiding this comment.
agreed that's what it seems like to me too
There was a problem hiding this comment.
Still don't think this is needed with the guard let self = self above; doesn't that accomplish what this does?
| @discardableResult | ||
| public func downloadData(for request: URLRequest, completion: @escaping CompletionHandler) -> SessionTaskProxyType? { | ||
| var proxy: SessionTaskProxyType? | ||
| let completionQueue = self.completionQueue ?? .current ?? .main |
There was a problem hiding this comment.
That's how it reads to me. Do we want to put this in the init?
|
|
||
| // Strongly capture self within the completion handler to ensure | ||
| // DataDownloader is persisted long enough to respond | ||
| let strongSelf = self |
There was a problem hiding this comment.
agreed that's what it seems like to me too
|
|
||
| // Strongly capture self within the completion handler to ensure | ||
| // DataDownloader is persisted long enough to respond | ||
| let strongSelf = self |
There was a problem hiding this comment.
Still don't think this is needed with the guard let self = self above; doesn't that accomplish what this does?
This pull request includes (pick all that apply):
Summary
The ImageDownloader and Cache works great but only supports images that are supported by UIImage/NSImage for the OS version of the device. If the Image doesn't support the data type, the data is lost and not cached, and we are unable to fully utilize the cache if we support other image types from our application.
URLDataCacheandDataDownloaderremoves the Image requirement from downloading the data needed while maintaining the functionality ofImageDownloaderand ImageCache.Implementation
In order to reduce duplicated code, most of the existing code from
URLImageCache.swiftandAutoPurgingURLImageCache.swiftand has been replaced withNSDatain place ofUIImageandNSImage. This allows for more flexibility in the data types being returned from dynamic URL types that we might not always have control over.NSDatawas chosen overDatain order to maintain the use ofNSCachewhich requires the use of a class [NSData], instead of a struct [Data]. Since these are interoperable, I felt this wouldn't be an issue.Test Plan
UIImage/NSImageis instantiated fromData, the cached data is lost.