chore: feature modules config by mrehan27 · Pull Request #413 · customerio/customerio-ios

mrehan27 · 2023-11-22T14:28:29Z

helps: https://github.com/customerio/issues/issues/11650

Changes

Removed SdkConfig usage from feature modules
Updated feature module config and utilized them in place of SdkConfig
Commented future changes with TODO and FIXME reminders

github-actions · 2023-11-22T14:28:44Z

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.

levibostian · 2023-11-22T15:22:02Z

+    mutating func apply(with dictionary: [String: Any]) {
+        // Each SDK config option should be able to be set from `dictionary`.
+        // If one isn't provided, use current value instead.
+
+        // If a parameter takes more logic to calculate, perform the logic up here.
+        if let regionStringValue = dictionary[Keys.region.rawValue] as? String {
+            self.region = Region.getRegion(from: regionStringValue)
+        }
+
+        // Construct object with all required parameters. Each config option should be updated from `dictionary` only if available.
+        if let siteId = dictionary[Keys.siteId.rawValue] as? String {
+            self.siteId = siteId
+        }
+    }


Suggested change

mutating func apply(with dictionary: [String: Any]) {

// Each SDK config option should be able to be set from `dictionary`.

// If one isn't provided, use current value instead.

// If a parameter takes more logic to calculate, perform the logic up here.

if let regionStringValue = dictionary[Keys.region.rawValue] as? String {

self.region = Region.getRegion(from: regionStringValue)

}

// Construct object with all required parameters. Each config option should be updated from `dictionary` only if available.

if let siteId = dictionary[Keys.siteId.rawValue] as? String {

self.siteId = siteId

}

}

internal static func init(from dictionary: [String: Any]) -> MessagingInAppConfigOptions? {

guard let regionStringValue = dictionary[Keys.region.rawValue] as? String, let region = Region.getRegion(from: regionStringValue) else {

return nil

}

guard let siteId = dictionary[Keys.siteId.rawValue] as? String else {

return nil

}

return MessagingInAppConfigOptions(siteId: siteId, region: region)

}

I suggest that instead of a mutating function, we take advantage of the struct constructor.

Why? Let's say that in the future we add a new property to the MessagingInAppConfigOptions struct, enable: Bool.

If we use a mutating function, we need to remember to go into that function and add support for this new enable property. If we take advantage of the struct constructor, the Swift compiler will not compile and remind us that line return MessagingInAppConfigOptions(siteId: siteId, region: region) is missing enable parameter.

I agree with this. Initially, I wanted to go with the constructor option for same reason, but avoided it due to uncertainty about how future changes in the native code and wrapper might be affected. Specifically, I wasn't sure if we need to initialize the configuration first and then update it, or if we can do both simultaneously.

Looking at this again, I realize this method was mainly added for wrappers, where we have more control over initializing the module. I'll switch to using the constructor, as it seems like a better option. If we encounter limitations that require early configuration initialization, we can reintroduce this method to overcome the problem.

levibostian · 2023-11-22T15:24:12Z

        guard let diGraph = diGraph else { return nil }

-        return MessagingInAppImplementation(diGraph: diGraph)
+        // FIXME: [CDP] Find workaround or reuse existing instance to utilize customer provided siteid


Idea that may or may not work: Would the journey's module replace the need for hooks for us?

If the journeys module allows the in-app module to be a consumer, the in-app module can get notified when a profile is identified or when a screen is tracked. Therefore, replacing what hooks is doing for us.

I wanted to share this if it's helpful to avoid this possibly complex FIXME scenario.

Honestly, I haven't given this much thought yet. But I agree that Journey being the bridge for feature modules communication with tracking/CDP would likely be the better choice for setting up hooks.

For this PR, I believe the only action item is to update the FIXME comment. We can then evaluate and make better decision once the Journey module is available, giving us a clearer picture.

That sounds like a great action item 👍

levibostian · 2023-11-22T15:27:53Z

        didReceive response: UNNotificationResponse
    ) -> CustomerIOParsedPushPayload? {
-        if sdkConfig.autoTrackPushEvents {
+        if moduleConfig.autoTrackPushEvents {


🤔 I think this could be a problem for us. That is assuming that we want SDK wrapper customers to be able to use Dart and JS to configure push features:

// RN example // in App.js file: let cioSdkConfig = ... cioSdkConfig.autoTrackPushEvents = false CustomerIO.initialize(cioSdkConfig)

See "Risks" section in this ticket where I try to explain this scenario and why this if statement living in the messaging push module would not work. Because this function would execute before RN/Flutter SDK initialize would execute.

This might mean that the messagingpush module can still accept the autoTrackPushEvents config property, but it might need to somehow pass that option to another module like journeys or CDP to handle this logic? Or the CustomerIO class holds this config option and passes it to the journeys module? Not sure.

This code here is code that we have in prod today. However, I know that one of our goals with this project is that config options live inside of feature modules. If we want autoTrackPushEvents to live inside of this module, that's why I am bringing this problem up.

Thanks for sharing Levi. I do agree with the problem. However, at this point, a quicker solution that I can think of for customers using wrapper SDKs might be to add something like the following in their AppDelegate / NotificationServiceExtension:

MessagingPush.shared.config { config in config.autoTrackPushEvents = false }

This obviously isn't final yet, and we'll continue to refine it as we work on the wrappers. But I think it could serve as a good short-term fix, while the ticket you shared addresses the problem in the long run.

Regarding passing to Journeys, I don’t believe this falls within the responsibility of the Journey module. The Push module should evaluate this and only pass the tracking event to Journeys if the flag is enabled. The autoTrackPushEvents flag is specific to the Push module. Journeys/CDP shouldn’t need to interact with this flag; if they receive a metric event, they should simply track it.

levibostian · 2023-11-22T15:30:27Z

+        case autoTrackDeviceAttributes
+    }
+
+    mutating func apply(with dictionary: [String: Any]) {


same comment about mutating function in the other config file.

Responded above

levibostian · 2023-11-22T15:37:55Z

        logger.info("push was sent from Customer.io. Processing the request...")

-        if sdkConfig.autoTrackPushEvents {
+        if moduleConfig.autoTrackPushEvents {


This if statement is OK to be here because we require all customers, including SDK wrapper customers, to initialize the native SDKs in the NSE file before this if statement runs. App lifecycle is not a problem for delivered metrics.

…nfig

…onfig

Shahroz16

Looks okay and none of the comments seems blockers. 👍

mrehan27 · 2023-11-27T10:36:58Z

Merging this to unblock myself for next PRs. We can continue the discussions and resolve them in upcoming PRs if needed

mrehan27 added 28 commits November 21, 2023 16:09

updated auto gen files

c69a054

added base protocol

d15fdf0

moved ModuleTopLevelObject class

2e643e0

push config

478e08f

inapp config

a093dbb

base object updates

d88fada

push updates

4527dc7

inapp updates

89256fe

removed config base

0b4cd23

added todo

75deed3

base class fix

c904ef6

added reminders

86cbb6f

unused imports

30e15a5

Merge branch 'shahroz/native-digraph' into rehan/cdp-module-wise-config

0eaa0ef

fix auto gen conflicts

b6ed987

updated deps

df3d744

remove imports

734a45a

update makefile

507b623

auto gen files

c1ef7ae

fixed more imports

3a88977

hooks update

d1a6714

added reminders

0a52400

updated Package.swift

cb172b7

push module config

502e893

reordering

e1bec23

inapp config

c644114

added apply functions

1837800

updated docs

2a0d912

mrehan27 self-assigned this Nov 22, 2023

levibostian reviewed Nov 22, 2023

View reviewed changes

mrehan27 added 7 commits November 24, 2023 20:37

inapp config

ba7748b

push config

fb0fa94

comment update

4b657c3

Merge branch 'shahroz/native-digraph' into rehan/cdp-module-deps

bfacb56

auto gen files

ac0d26d

comment update

2182e45

Merge branch 'rehan/cdp-module-deps' into rehan/cdp-feature-module-co…

fecd073

…nfig

mrehan27 marked this pull request as ready for review November 24, 2023 15:50

Base automatically changed from rehan/cdp-module-deps to shahroz/native-digraph November 27, 2023 07:43

Merge branch 'shahroz/native-digraph' into rehan/cdp-feature-module-c…

997e42e

…onfig

Shahroz16 approved these changes Nov 27, 2023

View reviewed changes

mrehan27 merged commit adc9f79 into shahroz/native-digraph Nov 27, 2023

mrehan27 deleted the rehan/cdp-feature-module-config branch November 27, 2023 10:37

Conversation

mrehan27 commented Nov 22, 2023

Changes

Uh oh!

github-actions Bot commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Sample app builds 📱

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shahroz16 left a comment

Choose a reason for hiding this comment

Uh oh!

mrehan27 commented Nov 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Nov 22, 2023 •

edited

Loading