-
Notifications
You must be signed in to change notification settings - Fork 392
PoC: allow overriding currencySymbol from a backend-provided ruleset #5663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Generated by 🚫 Danger |
4 builds increased size
RevenueCat 1.0 (1)
|
| Item | Install Size Change |
|---|---|
| DYLD.String Table | ⬆️ 50.9 kB |
| 📝 RevenueCat.CurrencySymbolOverridingPriceFormatter.CurrencySymbolO... | ⬆️ 4.8 kB |
| Code Signature | ⬆️ 3.7 kB |
| DYLD.Exports | ⬆️ 3.4 kB |
| 📝 RevenueCat.CurrencySymbolOverridingPriceFormatter.Objc Metadata | ⬆️ 1.3 kB |
RevenueCat 1.0 (1)
com.revenuecat.PaywallsTester.mac-catalyst-scaled-to-match-ipad
⚖️ Compare build
⏱️ Analyze build performance
Total install size change: ⬆️ 126.7 kB (0.29%)
Total download size change: ⬆️ 29.3 kB (0.24%)
Largest size changes
| Item | Install Size Change |
|---|---|
| DYLD.String Table | ⬆️ 45.3 kB |
| 📝 RevenueCat.CurrencySymbolOverridingPriceFormatter.CurrencySymbolO... | ⬆️ 4.6 kB |
| DYLD.Exports | ⬆️ 3.4 kB |
| Code Signature | ⬆️ 2.8 kB |
| 📝 RevenueCat.CurrencySymbolOverridingPriceFormatter.Objc Metadata | ⬆️ 1.2 kB |
RevenueCat 1.0 (1)
com.revenuecat.PaywallsTester.mac-catalyst-optimized-for-mac
⚖️ Compare build
⏱️ Analyze build performance
Total install size change: ⬆️ 126.7 kB (0.29%)
Total download size change: ⬆️ 29.3 kB (0.24%)
Largest size changes
| Item | Install Size Change |
|---|---|
| DYLD.String Table | ⬆️ 45.3 kB |
| 📝 RevenueCat.CurrencySymbolOverridingPriceFormatter.CurrencySymbolO... | ⬆️ 4.6 kB |
| DYLD.Exports | ⬆️ 3.4 kB |
| Code Signature | ⬆️ 2.8 kB |
| 📝 RevenueCat.CurrencySymbolOverridingPriceFormatter.Objc Metadata | ⬆️ 1.2 kB |
RevenueCat 1.0 (1)
com.revenuecat.PaywallsTester.mac-native
⚖️ Compare build
⏱️ Analyze build performance
Total install size change: ⬆️ 126.9 kB (0.33%)
Total download size change: ⬆️ 24.5 kB (0.23%)
Largest size changes
| Item | Install Size Change |
|---|---|
| DYLD.String Table | ⬆️ 45.3 kB |
| 📝 RevenueCat.CurrencySymbolOverridingPriceFormatter.CurrencySymbolO... | ⬆️ 4.6 kB |
| DYLD.Exports | ⬆️ 3.4 kB |
| Code Signature | ⬆️ 3.1 kB |
| 📝 RevenueCat.CurrencySymbolOverridingPriceFormatter.Objc Metadata | ⬆️ 1.2 kB |
🛸 Powered by Emerge Tools
Comment trigger: Size diff threshold of 100.00kB exceeded
📸 Snapshot Test7 modified, 868 unchanged
🛸 Powered by Emerge Tools |
| /// A `NumberFormatter` provider class for prices. | ||
| /// This provider caches the formatter to improve the performance. | ||
| final class PriceFormatterProvider: Sendable { | ||
| public final class PriceFormatterProvider: Sendable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably doesn't need to be public public if it's just RevenueCatUI using this, I'd probably add @_spi(Internal) so it's only used by RevenueCatUI
| public final class PriceFormatterProvider: Sendable { | |
| @_spi(Internal) public final class PriceFormatterProvider: Sendable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I don't think this will work, the SK2 product initializer in StoreProduct is public and has this type in the function signature. Will make sure to bring this up in the actual PR
| return formatter | ||
| } | ||
|
|
||
| if let formatter = formatter, formatter.currencyCode == currencyCode, formatter.locale == locale { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the logic here:
if let formatter = formatter as? CurrencySymbolOverridingPriceFormatter, formatter.currencyCode == currencyCode, formatter.locale == locale, formatter.currencySymbolOverride == currencySymbolOverride {
return formatter
}
if let formatter = formatter, formatter.currencyCode == currencyCode, formatter.locale == locale {
return formatter
}The first if checks if the formatter is a CurrencySymbolOverridingPriceFormatter and that all of the parameters match. If it fails, the second if still returns the formatter as long as the formatter's currency code & locale match.
Could this lead to a bug where if the formatter is a CurrencySymbolOverridingPriceFormatter, the overrides don't match, but the currency code & locale do, we return a CurrencySymbolOverridingPriceFormatter that doesn't match the current context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I think you're right. Addressed this :)
| } | ||
|
|
||
| private func formatter(for rule: PriceFormattingRuleSet.CurrencySymbolOverride.PluralRule) -> NumberFormatter { | ||
| if let formatter = numberFormatterCache[rule] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this 👏👏
| let placements: Placements? | ||
| let targeting: Targeting? | ||
| let uiConfig: UIConfig? | ||
| let config: Config? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: we should probably name this something more specific, like priceFormattingConfig, in case we decide to add other configs in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one, I figured we might want to use this as a 'config envelope' to also hold future configuration values, in order to avoid having to update all initializers and mocks again. But I will make sure to bring this up in my PR and ask for the team's opinion on this as well
d5403b6 to
7d6b087
Compare




We're having an issue related to currency formatting behavior in Romania (and possibly a few other currencies), internal discussion can be found here.
But some context: when using the Romanian Storefront, and a Romanian locale (ro_RO) prices are formatted as
1,23 RON, which is correct, but the more conventional way to write this in Romanian (according to reports) is1,23 lei.underlyingSK2Product.displayPriceuses the more commonleirather thanRON, however the priceFormatter we create inSK2StoreProduct.swiftthroughpriceFormatterProvider.priceFormatterForSK2based on the currency code and locale given to us by StoreKit creates strings usingRONinstead. This is causing discrepancies betweenlocalizedPriceStringand the price strings we generate using the formatter (for example pricePerMonth) which looks confusing on the paywall especially.After some research and discussions it looks like we can't get the NumberFormatter API to mimic the behavior of StoreKit, and thus we've been looking into an alternative approach.
We came up with a backend driven approach where we (given a ruleset) provide the option to override the currencySymbol for specific currency codes, given a storekit storefront (similar to what we already do for paywall localizations).
This PR is just a PoC outlining the rough workings of this approach. Happy to hear any feedback