[DISCUSSION][Tech Debt] - Fully Dynamic and Scalable Segment Integration

[ariefwijaya]

Hello, @danielgomezrico @pastuxso
just brainstorming, why don't we use this Integration Attribute as an array of enumeration or object list? It will be more clean, scalable and avoid breaking changes. I mean like, instead of using amplitudeIntegrationEnabled, mixpanelIntegrationEnabled. We can try using integrationList : [IntegrationType.mixpanel, IntegrationType.amplitude] or even use an object list integrationList : [AmplitudeIntegrationClass(enabled:true,param1:"value",param2:"value"),...]

wdyt all?

[ariefwijaya]

Just made PR#18 to work around this, Please check and test. I've tested it and I hope you'll be interested. @danielgomezrico

[danielgomezrico]

Thanks for the suggestion, I will open ideas in threads:

1. If we want to have an enum, we will need to have 3 enums with all possible values:

• One for Dart
• One for iOS
• One for Android

It worries to me that we need to keep all of them in sync, and also how we serialize that we send to each platform so each one could use an enum and not just a string and maybe have some human errors because of that

[danielgomezrico]

Another question:

2. The list of available services does not vary too much and even if you check the current values the enum just have one service, so having this feels a little overengineer something that is somehow static over time and having just bools is simpler, WDYT?

[ariefwijaya]

@danielgomezrico

1. I agree with that, For now, I'm creating enumerations in darts simply because I don't know a better way to map and create them in native iOS & Android. Please help me to do it and PR to my branch

2. I don't think so, there are so many integrations that can be used via Segments. Such as, mixpanel, moengage, firebase analytics/ google analytics, facebook, etc. (we can check it in their docs). My company uses a lot of platform integration with segments, that's how I learn that using boolean makes it too much boilerplate and not a good design. We need more dynamic and scalable stuff.

[danielgomezrico]

That's the thing, Im not sure whats the best way to have an enum, because as I said we would need to have 3 enums, one for each platform and serialize/deserialize the value on each one.

Do you have any other flutter library as an example that accepts enums and translate them on each platform? Just to have a reference?

[ariefwijaya]

@danielgomezrico
I don't think so, why do we need that? the values are exactly the same for each platform. Please check it on segment official docs

I have no reference to it. This is just to simplify the existing implementation. Because the current implementation causes more boilerplate and is not really needed.

[danielgomezrico]

It is not that easy to merge a breaking change At this moment because there are other PRs adding more integrations, and then they will break because of this change, like this one:

• feat: add option to activate native integration with appsflyer #19

We can take advantage that we have more integrations to see if having multiple of them this change does remove boilerplate or not, WDYT?

[ariefwijaya]

I think we don't need to worry about the breaking changes, those PR should be refactored after PR #18 merged. We need improvement before we are going further.

Anw, I've been using this branch with regards to new integrations and availability of other providers https://github.com/sribuu/flutter-segment/tree/sribuu-crm.

[danielgomezrico]

To continue the conversation, I found one integration that requires more information than just a enabled/disabled status to be enabled:

    let optlyLogger = OPTLYLoggerDefault(logLevel: .error)
    optlyManager = OPTLYManager.instance(builderBlock: { (builder) in
        builder?.projectId = "<YOUR_PROJECT_ID>"
        builder?.logger = optlyLogger
    })

    optlyManager?.initialize(callback: { (error, optlyClient) in
        // Optimizely is now up and running.  You can now configure any experiments, etc.
    })

    configuration.use(SEGOptimizelyXIntegrationFactory.instance(withOptimizely: optlyManager))

Docs: https://cocoapods.org/pods/Segment-Optimizely-X

It says to me that moving towards this approach will not cover all the cases, and maybe we should have something like a Config class for each case.

[cdmunoz]

@ariefwijaya what you are currently proposing from the engineering and architecture point of view is a really good and interesting stuff, it's really good to have such interesting contributions. Still has some nuts to tighten but it's indeed a really starting point to take into consideration.
Also we need to take into consideration how is the impact of one IC not only in the PR itself but also to the entire repository and this includes the other contributor's PRs.
The above in addition to last discussion comment from @danielgomezrico and to avoid a big breaking change that goes beyond the scope of the current goal of keeping the stuff as stable as possible, lead me to propose doing a pause in the PR #18 while others PRs are yet approved and while we could deepen all considerations with yours.
WDYT folks?

[ariefwijaya]

@danielgomezrico

Agreed, that one is not the only one. Those are more integration that requires more information than just enabled/disabled status. So, yes we need to have a config class for each integrations as I said before for e.g

IntegrationClass(enabled:true,param1:"value",param2:"value"),...]

Then, we need to have an abstract configuration class to extend for every plugin that needs it

[ariefwijaya]

Thanks @cdmunoz, for thoughtful feedback. I agreed with you, we need to consider more for this big change

[pastuxso]

@ariefwijaya Hi, I've resigned from this organization. Could you please remove my name from the description? 😉 👍 I'm clearing out my desk.