`Paywalls`: improve image caching #3498

NachoSoto · 2023-12-07T20:25:16Z

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.

NachoSoto · 2023-12-07T21:38:34Z

Tests/RevenueCatUITests/ImageLoaderTests.swift

+ expect(self.urlSession.cachePolicy) == .returnCacheDataElseLoad
+ }
+
+ func testLoadingImageResetsPreviousResult() async throws {


This test is pretty complicated but it was the simplest way I could confirm that the image is reset before loading the new one, without any race conditions.

makes sense. At least the code is very clear

NachoSoto · 2023-12-08T01:31:10Z

RevenueCatUI/Helpers/ImageLoader.swift

+
+ // Load images in a background thread
+ return await Task<Value, Never>
+ .detached(priority: .utility) { data.toImage() }


This is new

Feels like utility might be a bit low?

I'd imagined we'd have the pre-warming loader use utility or background and then ~~userInitiated~~ default when loading the paywall, given the potential impact on conversion for an image taking too long to load, and that there's probably not a lot more going on that's more important than the image at the time of displaying a paywall.

What do you think?

The pre-warming is actually happening with .default QOS because that's what OperationDispatcher.workerQueue is set up with :/
We should talk about changing that, maybe having multiple queues.

But I agree, I'll change this to default.

NachoSoto · 2023-12-08T01:31:24Z

RevenueCatUI/Helpers/ImageLoader.swift

+ return nil
+ }
+
+ if let httpResponse = response as? HTTPURLResponse, httpResponse.statusCode != 200 {


This is slightly different than the original implementation to allow responses for local files.

NachoSoto · 2023-12-08T01:31:38Z

RevenueCatUI/Helpers/ImageLoader.swift

+ .data(for: .init(url: url, cachePolicy: .returnCacheDataElseLoad))
+
+ do {
+ try Task.checkCancellation()


This allows preventing the image from being decoded if the task is already cancelled.

NachoSoto · 2023-12-08T01:31:54Z

RevenueCatUI/Helpers/ImageLoader.swift

+ #if os(macOS)
+ if let image = NSImage(data: self) {
+ return .success(.init(nsImage: image))
+ } else {
+ return .failure(.invalidImage)
+ }


We don't support macOS yet, but this makes the class compile, and it'll be ready when we do.

great catch, I keep forgetting macOS

NachoSoto · 2023-12-08T01:32:05Z

RevenueCatUI/Views/RemoteImage.swift

+ }
+ }
+ .transition(Self.transition)
+ .task(id: self.url) {


This automatically cancels the task when the url changes.

🤔 let's maybe add a comment? Feels like that would be easy to miss

It's the canonical way of implementing task: making the ID whatever parameter the task depends on. But yeah I'll add a comment 👍🏻

codecov · 2023-12-08T19:15:53Z

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (27138c4) 86.05% compared to head (cd6b37b) 86.01%.

Files	Patch %	Lines
Sources/Paywalls/PaywallCacheWarming.swift	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3498      +/-   ##
==========================================
- Coverage   86.05%   86.01%   -0.04%     
==========================================
  Files         237      237              
  Lines       17226    17231       +5     
==========================================
- Hits        14823    14821       -2     
- Misses       2403     2410       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aboedo · 2023-12-08T18:52:45Z

RevenueCatUI/Data/Strings.swift

@@ -16,6 +16,7 @@ import RevenueCat

 // swiftlint:disable identifier_name

+@available(iOS 13.0, macOS 10.15, tvOS 13.0, watchOS 6.0, *)


I realize it's not going to make a difference in practice, but our entire SDK is technically 6.2+ on watchOS, might as well set that as the min deployment target. Actually, cocoapods might complain if we don't? Not sure

and given that RCUI is 15+, maybe it'll complain further about that for all OS versions?

Oops thanks, not sure where I got this.

Oh this is because some method I needed from URLSession wasn't available until specifically these versions. So having a lower watchOS one doesn't matter, it's just overly available. But I'll update for consistency.

aboedo · 2023-12-08T19:43:30Z

RevenueCatUI/Helpers/ImageLoader.swift

+
+ // Load images in a background thread
+ return await Task<Value, Never>
+ .detached(priority: .utility) { data.toImage() }


Feels like utility might be a bit low?

I'd imagined we'd have the pre-warming loader use utility or background and then ~~userInitiated~~ default when loading the paywall, given the potential impact on conversion for an image taking too long to load, and that there's probably not a lot more going on that's more important than the image at the time of displaying a paywall.

What do you think?

aboedo · 2023-12-08T19:45:10Z

RevenueCatUI/Helpers/ImageLoader.swift

+ #if os(macOS)
+ if let image = NSImage(data: self) {
+ return .success(.init(nsImage: image))
+ } else {
+ return .failure(.invalidImage)
+ }


great catch, I keep forgetting macOS

aboedo · 2023-12-08T19:45:42Z

RevenueCatUI/Views/RemoteImage.swift

+ }
+ }
+ .transition(Self.transition)
+ .task(id: self.url) {


🤔 let's maybe add a comment? Feels like that would be easy to miss

aboedo · 2023-12-08T19:48:06Z

Tests/RevenueCatUITests/ImageLoaderTests.swift

+ expect(self.urlSession.cachePolicy) == .returnCacheDataElseLoad
+ }
+
+ func testLoadingImageResetsPreviousResult() async throws {


makes sense. At least the code is very clear

aboedo · 2023-12-08T19:48:45Z

couple of small questions, looking great though

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`.

NachoSoto · 2023-12-11T04:16:27Z

@aboedo all comments addressed.

tonidero

LGTM!

@NachoSoto

**This is an automatic release.** ### RevenueCatUI * `Paywalls`: improve image caching (#3498) via NachoSoto (@NachoSoto) * `Paywalls`: change style of CTA button to be consistent with other platforms (#3507) via NachoSoto (@NachoSoto) * `Paywalls`: open footer links on Safari on Catalyst (#3508) via NachoSoto (@NachoSoto) * `Paywalls`: fix compilation on Xcode < 14.3 (#3513) via NachoSoto (@NachoSoto) * Fix typo in zh-Hans localization of RevenueCatUI (#3512) via Francis Feng (@francisfeng) * `Paywalls`: fix PaywallViewControllerDelegate access from Objective-C (#3510) via noncenz (@noncenz) * `Paywalls`: open Terms and Privacy Policy links in-app (#3475) via Andy Boedo (@aboedo) * `Paywalls`: fix empty description when using `custom` package type and `{{ sub_period }}` (#3495) via Andy Boedo (@aboedo) * `Paywalls`: use `HEIC` images (#3496) via NachoSoto (@NachoSoto) * Paywalls: disable the close button when an action is in progress (#3474) via Andy Boedo (@aboedo) * `Paywalls`: adjusted German translations (#3476) via Tensei (@tensei) * Paywalls: Improve Chinese localization (#3489) via Andy Boedo (@aboedo) ### Other Changes * `CircleCI`: add git credentials to snapshot generation (#3506) via NachoSoto (@NachoSoto) * `StoreProduct`: improve `priceFormatter` documentation (#3500) via NachoSoto (@NachoSoto) * `Paywalls`: fix tests (#3499) via NachoSoto (@NachoSoto) * `Optimization`: changed `first` to `first(where:)` (#3467) via NachoSoto (@NachoSoto)

NachoSoto added perf pr:RevenueCatUI labels Dec 7, 2023

NachoSoto requested review from a team December 7, 2023 20:25

NachoSoto commented Dec 7, 2023

View reviewed changes

NachoSoto force-pushed the nacho/image-caching branch 2 times, most recently from 1fd50ad to bffe57d Compare December 7, 2023 21:41

NachoSoto commented Dec 8, 2023

View reviewed changes

NachoSoto mentioned this pull request Dec 8, 2023

Paywalls: explicitly cache images for paywalls #3477

Closed

NachoSoto force-pushed the nacho/image-caching branch from c44985b to cd6b37b Compare December 8, 2023 18:50

aboedo reviewed Dec 8, 2023

View reviewed changes

NachoSoto added 12 commits December 10, 2023 20:15

Paywalls: improve image caching

cfa6346

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`.

Load images in a background thread

00095dc

Reset image when loading a new URL

c024946

Remove import

2315fcc

Fix transition

1766596

Fix macOS

e859dff

Fix test

cea2ae3

Fix local requests

aeca7ca

Update snapshots

5598e85

Consistent watchOS availability

4884d40

Upgraded image loading priority

db78949

Added comment

d1761f4

NachoSoto force-pushed the nacho/image-caching branch from 23e99f2 to d1761f4 Compare December 11, 2023 04:16

NachoSoto requested a review from a team December 11, 2023 04:16

tonidero approved these changes Dec 11, 2023

View reviewed changes

NachoSoto requested a review from aboedo December 12, 2023 04:04

aboedo approved these changes Dec 12, 2023

View reviewed changes

NachoSoto merged commit 59a877a into main Dec 12, 2023
19 of 20 checks passed

NachoSoto deleted the nacho/image-caching branch December 12, 2023 15:24

RCGitBot mentioned this pull request Dec 12, 2023

[AUTOMATIC] Release/4.31.3 #3514

Merged

vegaro added pr:other and removed pr:perf labels Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Paywalls`: improve image caching #3498

`Paywalls`: improve image caching #3498

NachoSoto commented Dec 7, 2023

NachoSoto Dec 7, 2023

aboedo Dec 8, 2023

NachoSoto Dec 8, 2023

aboedo Dec 8, 2023

NachoSoto Dec 8, 2023

NachoSoto Dec 8, 2023

NachoSoto Dec 8, 2023

NachoSoto Dec 8, 2023

aboedo Dec 8, 2023

NachoSoto Dec 8, 2023

aboedo Dec 8, 2023

NachoSoto Dec 8, 2023

codecov bot commented Dec 8, 2023

aboedo Dec 8, 2023

aboedo Dec 8, 2023

NachoSoto Dec 8, 2023

NachoSoto Dec 8, 2023

aboedo Dec 8, 2023

aboedo Dec 8, 2023

aboedo Dec 8, 2023

aboedo Dec 8, 2023

aboedo commented Dec 8, 2023

NachoSoto commented Dec 11, 2023

tonidero left a comment

		@@ -16,6 +16,7 @@ import RevenueCat

		// swiftlint:disable identifier_name

		@available(iOS 13.0, macOS 10.15, tvOS 13.0, watchOS 6.0, *)

Paywalls: improve image caching #3498

Paywalls: improve image caching #3498

Conversation

NachoSoto commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 8, 2023

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboedo commented Dec 8, 2023

NachoSoto commented Dec 11, 2023

tonidero left a comment

Choose a reason for hiding this comment

`Paywalls`: improve image caching #3498

`Paywalls`: improve image caching #3498