-
Notifications
You must be signed in to change notification settings - Fork 23
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
chore: extract classes needed to be shared #408
chore: extract classes needed to be shared #408
Conversation
Sample app builds 📱Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## shahroz/cdp-native-refactor-base #408 +/- ##
====================================================================
- Coverage 58.17% 57.04% -1.13%
====================================================================
Files 123 125 +2
Lines 4727 4831 +104
====================================================================
+ Hits 2750 2756 +6
- Misses 1977 2075 +98 ☔ View full report in Codecov by Sentry. |
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.
I have some questions regarding this PR. I'll ask in Slack.
….com:customerio/customerio-ios into shahroz/native-digraph
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.
Looks good to me. I can see this has been resolved in Slack, so looks like we are good merging this.
d90ae74
into
shahroz/cdp-native-refactor-base
Third draft PR for [CDP] iOS Refactor: Base module#11645
Problem statement:
Current
DIGraph
is bound bysdkConfig
which is bound by SDK initializationPush and in-app should be able to access classes without being dependent on SDK being initialized
Push and in-app should be able to access classes that are not bound by the tracking module initialing the SDK
Removed the dependency on
SdkInitializedUtil
We are holding a strong reference to digraph just for this purposeBenefits:
Breaking sdkConfig to not be sandboxed and to be to utilize digraph would be a scope creep and a lot bigger change and this separate class removes that.
We have now added a concept of
scoped
graphs, previously everything was bound to sdkConfig scope hence the issue. Now we have a digraph that is not bound to anything, and we have a graph bound to sdkConfig and a template to add more graphs if needed for each module for the classes in that moduleEvents/Journeys
needs to be shared instance among all modules to make sure multiple instances aren't created and events are not missed.This approach makes sure that even if end up deciding we are going to keep tracking code (separate ticket), digraph or any of the older implementations doesn't break
We aren't modifying the
DiGraph
but rather adding another one, independent of sdkConfig that push and in-app are utilizing.