-
Notifications
You must be signed in to change notification settings - Fork 11
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: update config for data pipelines support #157
chore: update config for data pipelines support #157
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.
|
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, few comments
final InAppConfig? inAppConfig; | ||
final PushConfig pushConfig; | ||
|
||
CustomerIOConfig({ |
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 think trackApplicationLifecycleEvents
is not here as its part of another ticket, correct?
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 focused mainly on Dart file changes here. native SDK implementation and testing will be handled in the other ticket you mentioned.
this.cdnHost, | ||
this.flushAt, | ||
this.flushInterval, | ||
this.inAppConfig, |
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.
thinking our load, if a user has provided migrationSiteId
and not InAppConfig
should we automatically handle it for them?
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 initially thought the same, but doing it that way would mean losing flexibility in two scenarios:
- Disabling in-app messaging wouldn't be possible if
migrationSiteId
is provided - If a user creates a new
siteId
, it could cause issues sincemigrationSiteId
is supposed to remain the same, while in-app feature would require the newsiteId
Keeping them separate gives us better control in these cases.
closes: MBL-619
Changes
CustomerIOConfig
to align with Data Pipelines requirementsCustomerIOConfig
CustomerIOConfig
, removing redundant codeCustomerIOConfig
Code Changes
Before
After