Make ImageCache init private #665
Conversation
andrewdmontgomery
force-pushed
the
andrewdmontgomery/ImageCache-private-init
branch
from
250ed3a to
4bb042b
Compare
|
 |
App Name |  Gravatar Prototype Build |
| Build Number | 2123 | |
| Version | 1.0 | |
| Bundle ID | com.automattic.gravatar-sdk-demo-uikit.prototype-build | |
| Commit | 8424b6d | |
| App Center Build | Gravatar SDK Demo - UIKit #606 |
andrewdmontgomery
force-pushed
the
andrewdmontgomery/ImageCache-private-init
branch
from
4bb042b to
ef1f120
Compare
| @@ -2,50 +2,60 @@ import Gravatar | |||
| import UIKit | |||
|
|
|||
| package final class TestImageCache: ImageCaching, @unchecked Sendable { | |||
| let imageCache = ImageCache() | |||
| private var cache: [String: CacheEntry] = [:] | |||
There was a problem hiding this comment.
By using a Dictionary instead of the ImageCache.shared instance, we avoid having to make a bunch of private types package:
private class NSCacheForImageprivate final class CacheEntryObjectNSCacheextensionfileprivate subscript(_ key): String) -> CacheEntry?
|
|
||
| package typealias CacheMessage = (operation: CacheMessageType, key: String) | ||
| typealias CacheMessage = (operation: CacheMessageType, key: String?) |
There was a problem hiding this comment.
This compiles with internal access.
More importantly, I made key optional because there is no key used when clearing the cache.
| @@ -19,6 +19,9 @@ public protocol ImageCaching: Sendable { | |||
| /// `.inProgress(task)` is used by the image downloader to check if there's already an ongoing download task for the same image. If yes, the image | |||
| /// downloader awaits that ask instead of starting a new one. | |||
| func getEntry(with key: String) -> CacheEntry? | |||
|
|
|||
| /// Clears all entries from the cache | |||
| func clear() | |||
There was a problem hiding this comment.
AFAIU this is just added for the unit tests. We want to test the default implementation there which is the ImageCache struct. So I think we don't need to add a new method to the protocol just for unit testing purpose. 🤔
We can cast the shared instance to ImageCache and add clear() only to ImageCache.
Like:
let cache = try XCTUnwrap(ImageCache.shared as? ImageCache)
cache.clear()
There was a problem hiding this comment.
Ah shoot, I had meant to add a comment about this.
Yes, true, we only need it for testing in this PR. But I wondered if we wanted to make it clearable, generally. I know it's an in-memory cache. But are there times when we (or developers) might legitimately want to clear the cache without quitting the app?
I was thinking it would be handy during development.
There was a problem hiding this comment.
Oh wait, I talked too soon. Adding a new protocol method is a breaking change. So it would require a major release and we wouldn't want that just for this.
during development
During development is easy, ImageCache is a public struct, one can typecast the shared instance to it and call clear() that way.
There was a problem hiding this comment.
Adding a new protocol method is a breaking change. So it would require a major release and we wouldn't want that just for this.
That's a good point. Since it would be adding functionality (not breaking existing functionality), it would require a MINOR release.
I just removed that. But then it occurred to me, that the main purpose of this PR is to remove functionality. That breaks existing functionality. That requires a MAJOR release.
I think this PR should not target trunk. It should target our next major release.
There was a problem hiding this comment.
Ooh good point. I don't know why but I assumed the init() was was just internal. Yeah we can freeze this PR for now.
There was a problem hiding this comment.
That's a good point. Since it would be adding functionality (not breaking existing functionality), it would require a MINOR release.
Having said that, adding things to a "protocol" is not a new functionality I think, It's "demanding" new functionality. Like for example, Jetpack has its own image cache that implements this protocol. After this change it would generate a build error because Jetpack misses the implementation for this new method.
andrewdmontgomery
force-pushed
the
andrewdmontgomery/ImageCache-private-init
branch
from
ef1f120 to
4b8065c
Compare
Closes #388
Description
This PR makes the ImageCache init private. Since
TestImageCachewas using an ImageCache as its storage, I refactored it to use a dictionary.And since it now has its own caching logic, I thought it would be good to unit-test it.
Testing Steps