Paywalls: explicitly cache images for paywalls #3477
Conversation
|
this is ready to go from a code perspective... but I don't love the idea of not having any tests in it? Looks like our setup is mostly snapshot testing, which makes sense, but for stuff that is Paywalls-side logic it'd be great to have a test target. I'll make that as a separate PR if I get the time |
uhh wat
|
|
nevermind we do have tests and I even modified test cases earlier today, what am I saying |
|
going back to draft to write tests for it |
| struct RemoteImage: View { | ||
|
|
||
| @StateObject private var loader: ImageLoader | ||
| private let cache: URLCache |
There was a problem hiding this comment.
Small thing but do we need to store this here? Seems like this is unused.
|
Update here: we're actually supposed to be caching this stuff here https://github.com/RevenueCat/purchases-ios/blob/main/Sources/Paywalls/PaywallCacheWarming.swift, so we should dig into why that's not happening correctly before merging this, and unify the caching for both |
|
PaywallCacheWarming uses a different instance of URLCache, so we'll have to unify them. Also, we're using the shared cache, which isn't great because we're also setting the actual values for the cache in the whole app. A bit of a bigger refactor, though, since the warming happens in RC and the image loading currently happens in UI |
033b44c to
245cd29
Compare
NachoSoto
left a comment
There was a problem hiding this comment.
It's unfortunate that it means moving away from AsyncImage, but I guess it's time to finally implement this manually and make it work consistently.
|
|
||
| @Published var image: UIImage? | ||
| @Published var error: Error? | ||
| private var cancellable: AnyCancellable? |
There was a problem hiding this comment.
We're only storing one of these, but you can call load multiple times, so only the last one is cancelled?
There was a problem hiding this comment.
I'll update this so that overriding a cancellable cancels the previous one.
| @Published var image: UIImage? | ||
| @Published var error: Error? |
There was a problem hiding this comment.
Should this be a Result<UIImage, Error> instead?
There was a problem hiding this comment.
that'd make a lot more sense
| @available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *) | ||
| extension URLCache { | ||
|
|
||
| static let imageCache = URLCache(memoryCapacity: 5 * 1000 * 1000, diskCapacity: 30 * 1000 * 1000) |
There was a problem hiding this comment.
agreed! I commented the same thing
Supersedes #3477. This replaces `AsyncImage` with a custom implementation that allows us to ensure caching works reliably by explicitly using a custom `URLSession` with a shared `URLCache`.
|
Replaced by #3498 |
Supersedes #3477. This replaces `AsyncImage` with a custom implementation that allows us to ensure caching works reliably by explicitly using a custom `URLSession` with a shared `URLCache`.
Supersedes #3477. This replaces `AsyncImage` with a custom implementation that allows us to ensure caching works reliably by explicitly using a custom `URLSession` with a shared `URLCache`.
This adds explicit caching to images for Paywalls.
In my testing we seem to be mostly re-fetching images on every load. We're studying also improving the cache headers and that might (hopefully) render this PR unnecessary, but opening this up in the meantime just in case.
This PR fixes the re-fetching as far as I can tell.