chore: migrate classes to common #409
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.
|
| @@ -1,3 +1,4 @@ | |||
| @testable import CioInternalCommon | |||
There was a problem hiding this comment.
| @testable import CioInternalCommon |
I do understand why this import was added. Because CustomerIOInstance was moved into a different module, some parts of the codebase as well as customer's apps will be expected to find CustomerIOInstance and CustomerIOInstanceMock in Track module but will not be able to anymore.
It's not ideal for a customer to need to have to import internal modules into their code base so to get around that, we have these public aliases. I anticipate you will need to add CustomerIOInstance and CustomerIOInstanceMock to that file, too.
There was a problem hiding this comment.
some parts of the codebase as well as customer's apps will be expected to find CustomerIOInstance and CustomerIOInstanceMock in the Track module but will not be able to anymore.
I tried to cover it in this comment
Why would a customer be using CustomerIOInstance?
- We don't use it in our sample apps
- It's not part of our documentation
- None of the sample snippets ever shared with us include
CustomerIOInstance - It's not a general practice to use interface interface
CustomerIOInstanceinstead ofCustomerIOwhile adding an implementation.
The only usage that we currently have is testing as well, so that makes me think, that maybe we are worrying about something customers aren't even possibly using. What are your thoughts on it?
When we move the CustomerIO class to common, that's when we can have possible issues but I found this workaround that can probably be a good workaround for it
@_exported import CioInternalCommon
| func identify( | ||
| identifier: String, | ||
| body: [String: Any] | ||
| ) | ||
|
|
||
| // sourcery:Name=identifyEncodable | ||
| // sourcery:DuplicateMethod=identify | ||
| func identify<RequestBody: Encodable>( | ||
| identifier: String, | ||
| // sourcery:Type=AnyEncodable | ||
| // sourcery:TypeCast="AnyEncodable(body)" | ||
| body: RequestBody | ||
| ) | ||
|
|
||
| var registeredDeviceToken: String? { get } | ||
|
|
||
| func clearIdentify() | ||
|
|
||
| func track( | ||
| name: String, | ||
| data: [String: Any] | ||
| ) | ||
|
|
||
| // sourcery:Name=trackEncodable | ||
| // sourcery:DuplicateMethod=track | ||
| func track<RequestBody: Encodable>( | ||
| name: String, | ||
| // sourcery:Type=AnyEncodable | ||
| // sourcery:TypeCast="AnyEncodable(data)" | ||
| data: RequestBody? | ||
| ) | ||
|
|
||
| func screen( | ||
| name: String, | ||
| data: [String: Any] | ||
| ) | ||
|
|
||
| // sourcery:Name=screenEncodable | ||
| // sourcery:DuplicateMethod=screen | ||
| func screen<RequestBody: Encodable>( | ||
| name: String, | ||
| // sourcery:Type=AnyEncodable | ||
| // sourcery:TypeCast="AnyEncodable(data)" | ||
| data: RequestBody? | ||
| ) | ||
|
|
||
| var profileAttributes: [String: Any] { get set } | ||
| var deviceAttributes: [String: Any] { get set } | ||
|
|
||
| func registerDeviceToken(_ deviceToken: String) | ||
|
|
||
| func deleteDeviceToken() | ||
|
|
||
| func trackMetric( | ||
| deliveryID: String, | ||
| event: Metric, | ||
| deviceToken: String |
There was a problem hiding this comment.
🤔 I had an idea?
The reason for moving this interface into the Common module is for the CDP module to be able to add new public functions this interface (and I think CustomerIO class?).
These functions highlighted here I think are functions that are directly tied to the Track module, right?
Would it be worth having the CustomerIOInstance in the Common module be almost empty and then have the Track module add all of these functions via Track module?
This might be scope creep so I am not sure if it's worth doing now. This idea came to me because I do prefer where a design decision in the code is used throughout all of the code rather then little parts of the code here and there.
There was a problem hiding this comment.
That's a very good point, this interface does have indeed things that are tracking module only and they would indeed need to be moved into implementation by tracking modules.
I'll think a bit more about this to make sure, we are not adding redundancy while also adding the data pipeline specific extension. But i do see alot of value in keeping it bare minimum and letting track then add more specific implementation.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main-replica-for-cdp #409 +/- ##
=======================================================
Coverage ? 58.17%
=======================================================
Files ? 123
Lines ? 4727
Branches ? 0
=======================================================
Hits ? 2750
Misses ? 1977
Partials ? 0 ☔ View full report in Codecov by Sentry. |
|
Co-authored-by: Muhammad Rehan <[email protected]>
First draft PR for [CDP] iOS Refactor: Base module#11645
What it does is move
CustomerIOInstancetocommon.PR to follow next is what moves the
CustomerIOobject tocommon.