Skip to content

[Paywalls V2] Adds PaywallComponentsData#1935

Merged
JayShortway merged 89 commits into
mainfrom
pw2-paywallcomponentsdata
Nov 25, 2024
Merged

[Paywalls V2] Adds PaywallComponentsData#1935
JayShortway merged 89 commits into
mainfrom
pw2-paywallcomponentsdata

Conversation

@JayShortway

Copy link
Copy Markdown
Member

This PR adds:

  • PaywallComponentsData
  • ComponentsConfig
  • PaywallComponentsConfig
  • Background
  • LocalizationData with a custom serializer
  • tests

Comment on lines +38 to +44
// Documentation says to use either PrimitiveSerialDescriptor or buildClassSerialDescriptor. However, we need a
// polymorphic descriptor that's either a primitive (string) or a class. So neither of those options fit the bill.
@OptIn(InternalSerializationApi::class, ExperimentalSerializationApi::class)
override val descriptor: SerialDescriptor = buildSerialDescriptor(
serialName = "LocalizationData",
kind = PolymorphicKind.SEALED,
)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the feeling that kotlinx.serialization doesn't quite "agree" with what we're doing, which is the same JSON key sometimes pointing to a primitive (string) and sometimes to a class (ThemeImageUrls). That's why I had to add these annotations to make it work.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhhh yeaaah, I can see this not working well in some places 😬 It felt surpringly well in Swift but 🤔 I can't see us adding more than just text or images though. Do you think this is still okay to go this way?

@JayShortway JayShortway Nov 25, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding a wrapper object ({ }) would be safer, since we have to resort to using an InternalSerializationApi here. This should work fine for the current kotlinx.serialization version, but it might break in future versions. If that happens, the amount of effort required to make it work again is hard to say right now (but > 0).

It doesn't look like we will be blocked if they remove the InternalSerializationApi, as we could always implement SerialDescriptor ourselves (although that, in turn, is marked as unstable). But you never know.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, we'll explore changing the schema for this. I'm going to merge this now and will update the SDK separately.

@emerge-tools

emerge-tools Bot commented Nov 22, 2024

Copy link
Copy Markdown

📸 Snapshot Test

74 unchanged

Name Added Removed Modified Unchanged Errored Approval
TestPurchasesUIAndroidCompatibility
com.revenuecat.testpurchasesuiandroidcompatibility
0 0 0 74 0 N/A

🛸 Powered by Emerge Tools

@joshdholtz joshdholtz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Comment on lines +38 to +44
// Documentation says to use either PrimitiveSerialDescriptor or buildClassSerialDescriptor. However, we need a
// polymorphic descriptor that's either a primitive (string) or a class. So neither of those options fit the bill.
@OptIn(InternalSerializationApi::class, ExperimentalSerializationApi::class)
override val descriptor: SerialDescriptor = buildSerialDescriptor(
serialName = "LocalizationData",
kind = PolymorphicKind.SEALED,
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhhh yeaaah, I can see this not working well in some places 😬 It felt surpringly well in Swift but 🤔 I can't see us adding more than just text or images though. Do you think this is still okay to go this way?

Base automatically changed from pw2-stickyfootercomponent to main November 25, 2024 13:35
@codecov

codecov Bot commented Nov 25, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 61.53846% with 15 lines in your changes missing coverage. Please review.

Project coverage is 81.93%. Comparing base (1bf7664) to head (3176104).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...hases/paywalls/components/PaywallComponentsData.kt 46.15% 6 Missing and 1 partial ⚠️
...ecat/purchases/paywalls/components/Localization.kt 73.33% 4 Missing ⚠️
...nuecat/purchases/paywalls/components/Background.kt 60.00% 0 Missing and 2 partials ⚠️
.../purchases/paywalls/components/ComponentsConfig.kt 66.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1935      +/-   ##
==========================================
- Coverage   82.03%   81.93%   -0.10%     
==========================================
  Files         256      260       +4     
  Lines        8371     8410      +39     
  Branches     1207     1215       +8     
==========================================
+ Hits         6867     6891      +24     
- Misses       1011     1021      +10     
- Partials      493      498       +5     

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

@JayShortway JayShortway merged commit 8cb7bdb into main Nov 25, 2024
@JayShortway JayShortway deleted the pw2-paywallcomponentsdata branch November 25, 2024 19:13
This was referenced Nov 28, 2024
JayShortway pushed a commit that referenced this pull request Dec 2, 2024
**This is an automatic release.**

## RevenueCat SDK
### 🐞 Bugfixes
* Uses `Sequence` instead of `Stream` to avoid errors due to unavailable
Java 8 APIs (#1943) via JayShortway (@JayShortway)

### 🔄 Other Changes
* Increase integration test timeout (#1946) via Toni Rico (@tonidero)
* Removes `@RequiresApi(N)` from `FileHelper` and related classes
(#1944) via JayShortway (@JayShortway)
* [Paywalls V2] Minimizes Java API (#1942) via JayShortway
(@JayShortway)
* [Paywalls V2] Makes `TextComponent` public (#1939) via JayShortway
(@JayShortway)
* Introduces an `@InternalRevenueCatAPI` annotation (#1938) via
JayShortway (@JayShortway)
* [Paywalls V2] Moves any non-component file to a new `common` package.
(#1937) via JayShortway (@JayShortway)
* [Paywalls V2] `LocalizationKey` is an inline value class now. (#1936)
via JayShortway (@JayShortway)
* [Paywalls V2] Adds `PaywallComponentsData` (#1935) via JayShortway
(@JayShortway)
* [Paywalls V2] Adds `StickyFooterComponent` (#1934) via JayShortway
(@JayShortway)
* [Paywalls V2] Adds `PurchaseButtonComponent` (#1933) via JayShortway
(@JayShortway)
* [Paywalls V2] Adds `PackageComponent` (#1932) via JayShortway
(@JayShortway)
* Ensure the correct error message is shown when failing to open a Uri
in paywalls (#1922) via JayShortway (@JayShortway)
* [Paywalls V2] Adds `ButtonComponent` (#1931) via JayShortway
(@JayShortway)
* [Paywalls V2] Adds `StackComponent` (#1930) via JayShortway
(@JayShortway)
* [Paywalls V2] Adds `ComponentOverrides` (#1929) via JayShortway
(@JayShortway)
* [Paywalls V2] Adds `ImageComponent` (#1928) via JayShortway
(@JayShortway)
* [Paywalls V2] Adds `TextComponent` (#1927) via JayShortway
(@JayShortway)
* [Paywalls V2] Adds all enum properties (#1926) via JayShortway
(@JayShortway)
* [Paywalls V2] Adds `SizeConstraints`, `Size`, `Padding` and `Shadow`
properties (#1925) via JayShortway (@JayShortway)
* [Paywalls V2] Adds `CornerRadiuses`, `Shape` and `MaskShape`
properties (#1924) via JayShortway (@JayShortway)
* [Paywalls V2] Adds `ImageUrls` and `ColorInfo` properties (#1923) via
JayShortway (@JayShortway)

Co-authored-by: revenuecat-ops <ops@revenuecat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants