chore: separate data pipeline module #415

Shahroz16 · 2023-11-27T12:03:44Z

PR for [CDP] iOS Native Refactor: Creation of Data-pipeline module#11646

Goal:
Have a separate module for Segment implementation Data Pipeline that we can utilize from Tracking and extension methods on the CustomerIO class, that customers can call.

….com:customerio/customerio-ios into shahroz/native-digraph

…igraph

…erio-ios into shahroz/datapipeline-module

…io-ios into shahroz/datapipeline-module

github-actions · 2023-11-27T12:03:58Z

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.

CocoaPods-FCM: Build failed. See CI job logs to determine the issue and try re-building.
APN-UIKit: Build failed. See CI job logs to determine the issue and try re-building.

Shahroz16 · 2023-11-27T12:04:33Z

Package.swift

@@ -41,7 +42,8 @@ let package = Package(
        // https://web.archive.org/web/20220525200227/https://www.timc.dev/posts/understanding-swift-packages/
        //
        // Update to exact version until wrapper SDKs become part of testing pipeline.
-        .package(name: "Firebase", url: "https://github.com/firebase/firebase-ios-sdk.git", "8.7.0"..<"11.0.0")
+        .package(name: "Firebase", url: "https://github.com/firebase/firebase-ios-sdk.git", "8.7.0"..<"11.0.0"),
+        .package(name: "Segment", url: "https://github.com/segmentio/analytics-swift.git", .branch("main"))


This will be changed to the forked version, rest should be the same

Shahroz16 · 2023-11-27T12:06:45Z

Sources/DataPipeline/CIODataPipelineConfig.swift

+import Segment
+
+extension Configuration {
+    static var defaultConfiguration: Configuration {


This is dependent on the DIGraph work, so we can get either the write key or site id or API key to have the config initialized as default.

Good call-out.

Could you add a TODO or FIXME comment on this line stating that this file is going to change after some other PR is created?

Shahroz16 · 2023-11-27T12:08:15Z

Sources/DataPipeline/CIODataPipeline.swift

+import Segment
+
+public enum CIODataPipeline {
+    private static var analytics: Analytics = .init(configuration: Configuration.defaultConfiguration)


analytics object can't be optional, as Segment always expect it be non nullable. CIODataPipeline will be initialized from CustomerIOImplementation on SDK launch so there shouldn't be a case where this would be nullable.

levibostian · 2023-11-27T16:09:53Z

Package.swift

@@ -16,6 +16,7 @@ import Foundation
 // Therefore, it's important that we only expose modules that we want customers to use. Internal modules should not be included in this array.
 var products: [PackageDescription.Product] = [
    .library(name: "Tracking", targets: ["CioTracking"]),
+    .library(name: "DataPipeline", targets: ["CioDataPipeline"]),


Suggested change

.library(name: "DataPipeline", targets: ["CioDataPipeline"]),

.library(name: "DataPipeline", targets: ["CioDataPipelines"]),

Somewhat nit-picky, but important to me since it's customer facing.

Since we call the product "Data Pipelines" I think it makes sense for the public SDK module name to match that naming.

levibostian · 2023-11-27T16:10:53Z

Package.swift

@@ -16,6 +16,7 @@ import Foundation
 // Therefore, it's important that we only expose modules that we want customers to use. Internal modules should not be included in this array.


Friendly reminder to also update cocoapods files.

You will probably need to create a new podspec file, update CustomerIO.podspec file, and also update the CI cocoapods deployments script to deploy the new podspec file.

Good call-out.

levibostian · 2023-11-27T16:12:54Z

Sources/DataPipeline/CIODataPipeline.swift

+    private init() {}
+
+    // Static method to access the singleton Analytics instance.
+    public static func shared() -> Analytics {


I was expecting CIODataPipeline.shared to return an instance of CIODataPipeline but this is returning the Segment SDK instance.

Do we need to publicly expose the Segment SDK? If so, is shared the right name for exposing it?

if we are to use the instance of Analytics in extension method of CustomerIO we need to have it as a static member of CIODataPipeline.

Open to naming it whatever makes the most sense, since this is not going to be accessed by the users directly, shared seemed like a good candidate.

shared is a commonly used Swift name when naming singleton instances. I think that is what is throwing me off. If I saw Swift code CIODataPipeline.shared, I was assume that an instance of CIODataPipeline would be returned.

Instead of having this shared function, could we instead re-use the static Segment variable above?

public private(set) static var analytics: Analytics = .init(configuration: Configuration.defaultConfiguration)

Then, CIODataPipeline.analytics would do the same thing as CIODataPipeline.shared does currently.

levibostian · 2023-11-27T16:17:54Z

Sources/DataPipeline/CIODataPipeline.swift

+
+// MARK: - System Modifiers
+
+public extension CustomerIO {


I am assuming that all of these extensions are added for Segment drop-in requirement?

If so, I think it would be good to move this extension block into another file to keep the drop-in code encapsulated somewhere.

One idea is using a common Swift pattern of using the file name to indicate it's purpose. Perhaps creating a new file CustomerIO+Segment.swift and putting this block of code inside could be helpful?

Yup that's correct, and that's a very good suggestion.

levibostian · 2023-11-27T16:19:32Z

Sources/DataPipeline/CIODataPipeline.swift

+
+    /// Retrieve the version of this library in use.
+    /// - Returns: A string representing the version in "BREAKING.FEATURE.FIX" format.
+    func version() -> String {


Do we want to expose the Segment SDK version?

It feels weird to me for a customer to call CustomerIO.version and CIODataPipeline.version and get 2 different returned results? Also feels weird to expose the version of a dependency that our SDK has?

I just feature matched the segments APIs, we can drop/keep or make any decisions in the ready for production tickets, otherwise the discussion might scope creep the PRs. What do you think?

I am OK with deciding later.

Idea: We make a decision in project slack channel? This seems like a short and quick decision to me. Could be answered async together.

levibostian · 2023-11-27T16:22:35Z

Sources/DataPipeline/CIODataPipelineConfig.swift

+import Segment
+
+extension Configuration {
+    static var defaultConfiguration: Configuration {


Good call-out.

Could you add a TODO or FIXME comment on this line stating that this file is going to change after some other PR is created?

levibostian · 2023-11-27T16:23:34Z

Sources/DataPipeline/CIODataPipelineEvents.swift

+    // make a note in the docs on this that we removed the old "options" property
+    // and they need to write a middleware/enrichment now.
+    // the objc version should accomodate them if it's really needed.


Suggested change

// make a note in the docs on this that we removed the old "options" property

// and they need to write a middleware/enrichment now.

// the objc version should accomodate them if it's really needed.

// TODO: make a note in the docs on this that we removed the old "options" property

// and they need to write a middleware/enrichment now.

// the objc version should accomodate them if it's really needed.

Is this comment still valid? If so, I suggest adding TODO to make it easier to find and fix in a future PR.

This is the comment from Segment about this method, i copied it right as is so we can make a more informed decision later.

Ok. If it's a comment for their codebase and not ours, I would consider removing it in this PR. To avoid confusing someone in the future like it did for me.

This is a non-blocker and you can decide what you think is best regarding what to do with this.

levibostian · 2023-11-27T16:26:20Z

Sources/DataPipeline/CIODataPipelineEvents.swift

+
+// MARK: - Typed Event Signatures
+
+public extension CustomerIO {


I think it would be a good idea to create an APITest for this new module. It has been really helpful to make sure that the public-facing API compiles for customers as intended.

If you agree, could you make sure this is added as a task to a CDP ticket to do this? It's OK if this is not part of this PR if that's out of scope.

Agreed, I was thinking it could be added to Ready for Production tickets where we can add even more test cases related.

I am OK with that.

If that's the case, could you add a // TODO or // FIXME comment to this file specifying that we are planning on that?

Shahroz16 added 23 commits November 14, 2023 23:29

move interface to common

2c4c02f

fix tests

4acb489

autogen files update

c5dd664

added a separate module

2a6e2ed

added global digraph

9052f6c

added missing autogen deps

aa51e7c

more sourcery

cd0c93f

auto gen

eab1a51

new annotations

dde26d7

updated shared graph classes

eeec59b

updated files

f4f59e5

mocking test fixed

49b127e

auto

cc1345b

migrated customer io to common

0ff2b8b

Merge branch 'shahroz/cdp-native-refactor-customerio-class' of github…

bd93ebf

….com:customerio/customerio-ios into shahroz/native-digraph

chore: migrated customerio to common (#411)

0528873

Merge branch 'shahroz/cdp-native-refactor-base' into shahroz/native-d…

db961f3

…igraph

Merge branch 'shahroz/native-digraph' of github.com:customerio/custom…

602b8d1

…erio-ios into shahroz/datapipeline-module

auto gen

3f6561d

added the package

e1f78af

added the wrapper methods

8061535

Merge branch 'main-replica-for-cdp' of github.com:customerio/customer…

1bd7e9d

…io-ios into shahroz/datapipeline-module

merge issue

5829181

Shahroz16 added 2 commits November 27, 2023 17:05

merge issue

89d9251

converted to class

f3c56a4

Shahroz16 commented Nov 27, 2023

View reviewed changes

levibostian reviewed Nov 27, 2023

View reviewed changes

levibostian approved these changes Nov 27, 2023

View reviewed changes

updated module name

2dcd944

Shahroz16 added 3 commits November 28, 2023 17:53

pr suggestion

28ff331

missed dependency in tracking

9decb4c

pr changes

4eaaae9

Shahroz16 marked this pull request as ready for review November 29, 2023 10:35

Shahroz16 merged commit 2c8f1d7 into main-replica-for-cdp Nov 29, 2023

Shahroz16 deleted the shahroz/datapipeline-module branch November 29, 2023 10:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: separate data pipeline module #415

chore: separate data pipeline module #415

Shahroz16 commented Nov 27, 2023 •

edited

Loading

github-actions bot commented Nov 27, 2023 •

edited

Loading

Shahroz16 Nov 27, 2023

Shahroz16 Nov 27, 2023

levibostian Nov 27, 2023

Shahroz16 Nov 27, 2023

levibostian Nov 27, 2023

levibostian Nov 27, 2023

Shahroz16 Nov 28, 2023

levibostian Nov 27, 2023

Shahroz16 Nov 28, 2023

levibostian Nov 28, 2023

levibostian Nov 27, 2023

Shahroz16 Nov 28, 2023

levibostian Nov 27, 2023

Shahroz16 Nov 28, 2023

levibostian Nov 28, 2023

levibostian Nov 27, 2023

levibostian Nov 27, 2023

Shahroz16 Nov 28, 2023 •

edited

Loading

levibostian Nov 28, 2023

levibostian Nov 27, 2023

Shahroz16 Nov 28, 2023

levibostian Nov 28, 2023

	.library(name: "DataPipeline", targets: ["CioDataPipeline"]),
	.library(name: "DataPipeline", targets: ["CioDataPipelines"]),

		@@ -16,6 +16,7 @@ import Foundation
		// Therefore, it's important that we only expose modules that we want customers to use. Internal modules should not be included in this array.


		// MARK: - Typed Event Signatures

		public extension CustomerIO {

chore: separate data pipeline module #415

chore: separate data pipeline module #415

Conversation

Shahroz16 commented Nov 27, 2023 • edited Loading

github-actions bot commented Nov 27, 2023 • edited Loading

Sample app builds 📱

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shahroz16 Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shahroz16 commented Nov 27, 2023 •

edited

Loading

github-actions bot commented Nov 27, 2023 •

edited

Loading

Shahroz16 Nov 28, 2023 •

edited

Loading