Create URLDataCache and DataDownloader by anthony-lipscomb-dev · Pull Request #164 · mindbody/Conduit

anthony-lipscomb-dev · 2021-08-02T22:40:51Z

I've read, understood, and done my best to follow the CONTRIBUTING guidelines.

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.

URLDataCache and DataDownloader removes the Image requirement from downloading the data needed while maintaining the functionality of ImageDownloader and ImageCache.

Implementation

In order to reduce duplicated code, most of the existing code from URLImageCache.swift and AutoPurgingURLImageCache.swift and has been replaced with NSData in place of UIImage and NSImage. This allows for more flexibility in the data types being returned from dynamic URL types that we might not always have control over.

NSData was chosen over Data in order to maintain the use of NSCache which 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

Ensure Images are still downloaded, cached, and retrieved properly. This can be done using the tests or in your application. I have used Proxyman to determine that multiple requests were not being made by the application to request subsequent Images from the web. In prior versions these requests would continue to be made because once UIImage/NSImage is instantiated from Data, the cached data is lost.
[New functionality] Ensure unsupported image types from the web are being stored properly. This can be done using the tests or in your application. I have used Proxyman to determine that multiple requests were not being made by the application to request subsequent Images from the web.

bconway99 · 2021-08-04T14:20:47Z

Sources/Conduit/Networking/Data/DataDownloader.swift

+    @discardableResult
+    public func downloadData(for request: URLRequest, completion: @escaping CompletionHandler) -> SessionTaskProxyType? {
+        var proxy: SessionTaskProxyType?
+        let completionQueue = self.completionQueue ?? .current ?? .main


So we could pass in a background queue to download data. But if left nil it'll default to current or main?

That's how it reads to me. Do we want to put this in the init?

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

Sources/Conduit/Networking/Data/DataDownloader.swift

bconway99 · 2021-08-04T14:24:27Z

Sources/Conduit/Networking/Data/DataDownloader.swift

+                    strongSelf.sessionProxyMap[cacheIdentifier] = nil
+                    strongSelf.completionHandlerMap[cacheIdentifier]?.forEach(execute)
+                    strongSelf.completionHandlerMap[cacheIdentifier] = nil
+                }


Could you please explain the need for intentional retain cycles a bit more?

@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

bconway99 · 2021-08-04T14:29:37Z

Tests/ConduitTests/Networking/Data/DataDownloaderTests.swift

+
+        waitForExpectations(timeout: 5)
+    }
+


Looks great, I'd just recommend adding the "GIVEN, WHEN, THEN" statements to the above tests as well.

bconway99

Looking great just a couple of small comments.

tiltem · 2021-08-04T15:45:02Z

Sources/Conduit/Networking/Data/AutoPurgingURLDataCache.swift

+        return _data(for: request)
+    }
+
+    private func _data(for request: URLRequest) -> NSData? {


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?

Sources/Conduit/Networking/Data/DataDownloader.swift

tiltem · 2021-08-04T15:48:08Z

Sources/Conduit/Networking/Data/DataDownloader.swift

+
+            // Strongly capture self within the completion handler to ensure
+            // DataDownloader is persisted long enough to respond
+            let strongSelf = self


with the above guard statement (line 79) defined as it is self here is non-optional unless I'm misreading the scopes

agreed that's what it seems like to me too

Still don't think this is needed with the guard let self = self above; doesn't that accomplish what this does?

plflanagan

Great work. I've left a few questions/comments throughout 👍

Sources/Conduit/Networking/Data/AutoPurgingURLDataCache.swift

plflanagan · 2021-08-04T16:53:53Z

Sources/Conduit/Networking/Data/DataDownloader.swift

+    @discardableResult
+    public func downloadData(for request: URLRequest, completion: @escaping CompletionHandler) -> SessionTaskProxyType? {
+        var proxy: SessionTaskProxyType?
+        let completionQueue = self.completionQueue ?? .current ?? .main


That's how it reads to me. Do we want to put this in the init?

Sources/Conduit/Networking/Data/DataDownloader.swift

plflanagan · 2021-08-04T17:02:32Z

Sources/Conduit/Networking/Data/DataDownloader.swift

+
+            // Strongly capture self within the completion handler to ensure
+            // DataDownloader is persisted long enough to respond
+            let strongSelf = self


agreed that's what it seems like to me too

Tests/ConduitTests/Networking/Data/AutoPurgingURLDataCacheTests.swift

Tests/ConduitTests/Networking/Data/DataDownloaderTests.swift

plflanagan · 2021-08-05T22:07:15Z

Sources/Conduit/Networking/Data/DataDownloader.swift

+
+            // Strongly capture self within the completion handler to ensure
+            // DataDownloader is persisted long enough to respond
+            let strongSelf = self


Still don't think this is needed with the guard let self = self above; doesn't that accomplish what this does?

anthony-lipscomb-dev added 2 commits August 2, 2021 15:13

Create DataDownloader and URLDataCache

aacefb7

public AutoPurgingURLDataCache

8c8beb5

anthony-lipscomb-dev added the enhancement label Aug 2, 2021

anthony-lipscomb-dev self-assigned this Aug 2, 2021

anthony-lipscomb-dev closed this Aug 2, 2021

add tests

986f94f

anthony-lipscomb-dev reopened this Aug 4, 2021

bconway99 reviewed Aug 4, 2021

View reviewed changes

Sources/Conduit/Networking/Data/DataDownloader.swift Show resolved Hide resolved

bconway99 reviewed Aug 4, 2021

View reviewed changes

bconway99 approved these changes Aug 4, 2021

View reviewed changes

tiltem reviewed Aug 4, 2021

View reviewed changes

plflanagan reviewed Aug 4, 2021

View reviewed changes

Fix tests, remove self references where not needed

597db64

tiltem approved these changes Aug 5, 2021

View reviewed changes

plflanagan approved these changes Aug 5, 2021

View reviewed changes

fix tests

b2a3c9f

anthony-lipscomb-dev merged commit 44611d6 into main Aug 6, 2021

anthony-lipscomb-dev deleted the task/create-data-downloader branch August 6, 2021 00:52

Conversation

anthony-lipscomb-dev commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Implementation

Test Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bconway99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plflanagan Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plflanagan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plflanagan Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anthony-lipscomb-dev commented Aug 2, 2021 •

edited

Loading

plflanagan Aug 5, 2021 •

edited

Loading

plflanagan Aug 5, 2021 •

edited

Loading