refactor: Introduce EventsManager to track events for different features#2096
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2096 +/- ##
==========================================
- Coverage 81.11% 80.81% -0.30%
==========================================
Files 271 272 +1
Lines 8953 9049 +96
Branches 1269 1280 +11
==========================================
+ Hits 7262 7313 +51
- Misses 1162 1201 +39
- Partials 529 535 +6 ☔ View full report in Codecov by Sentry. |
|
@RevenueCat/coresdk one thing I am thinking is that maybe its worth is keeping PaywallsEventManager instead of the legacy FileHelper because it's already tested. Either that, or just keep the tests from PaywallsManager instead EventsManagerTests |
| subclass(BackendEvent.Paywalls::class, BackendEvent.Paywalls.serializer()) | ||
| } | ||
| } | ||
| classDiscriminator = "discriminator" |
There was a problem hiding this comment.
I thought in the request there's no discriminator?
There was a problem hiding this comment.
I ended up adding it because tests were failing. If I understood correctly, you must use a classDiscriminator when using polymorphic. However, this is just use for tests and it should not break anything, since it's just an extra field right?
vegaro
left a comment
There was a problem hiding this comment.
I think this should be good to merge. Just some small comments
| flushInProgress = true | ||
|
|
||
| if (!legacyFlushDone) { | ||
| flushLegacyEvents() |
There was a problem hiding this comment.
actually since it's enqueued... there's a race condition if flushEvents executes again before flushLegacyEvents finishes? A way to solve it is removing the enqueuing in flushLegacyEvents. Or actually... shouldn't it go before flushLegacyEvents and not after?
| * | ||
| * @property event The associated `PaywallEvent`. | ||
| */ | ||
| data class Paywall(val event: PaywallEvent) : FeatureEvent() |
There was a problem hiding this comment.
not a merge stopper but this could be directly the PaywallEvent? So you could make the PaywallEvent a FeatureEvent
There was a problem hiding this comment.
I can apply this in another PR, since this has already grown a lot 😓
Motivation
Introduce events for Customer Center
Description
I am working on introducing events for customer center, and I needed an event manager an realized PaywallEventManager could be generalized to be reused. This introduces EventsManager, with backwards compatibility for previous events