Skip to content
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

refactor: push modules initialization #418

Merged
merged 68 commits into from
Dec 4, 2023

Conversation

mrehan27
Copy link
Contributor

@mrehan27 mrehan27 commented Nov 30, 2023

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

Changes

  • APN-UIKit AppDelegate fix for MessagingInApp
  • Updated MessagingPush to initialize only once
  • Removed configure option from MessagingPush
  • Updated MessagingPushAPN and MessagingPushFCM initialization methods
  • Fixed some tests so they compile

@mrehan27 mrehan27 self-assigned this Nov 30, 2023
Copy link

github-actions bot commented Nov 30, 2023

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: rehan/cdp-push-module (1701440127)

@@ -7,10 +7,10 @@ import Foundation
*/
public class MessagingPush: ModuleTopLevelObject<MessagingPushInstance>, MessagingPushInstance {
@Atomic public private(set) static var shared = MessagingPush()
@Atomic public private(set) static var moduleConfig: MessagingPushConfigOptions = MessagingPushConfigOptions.Factory.create()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we've already implemented this in the upcoming changes for improving push metrics, I've reused the same to avoid introducing another option for the same functionality.

Comment on lines +50 to 52
guard getImplementationInstance() == nil else {
logger.info("\(moduleName) module is already initialized. Ignoring redundant initialization request.")
return
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on our ongoing slack discussion

@mrehan27 mrehan27 requested a review from a team November 30, 2023 15:29
logger.info("\(moduleName) module successfully set up with SDK")
}

override public func createImplementationInstance() -> MessagingPushInstance? {
MessagingPush.initialize()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To disable auto initialization for MessagingPush

Copy link

github-actions bot commented Dec 1, 2023

Warnings
⚠️

I noticed file Tests/MessagingInApp/APITest.swift was modified. That could mean that this pull request is introducing a breaking change to the SDK.

If this pull request does introduce a breaking change, make sure the pull request title is in the format:

<type>!: description of breaking change 
// Example:
refactor!: remove onComplete callback from async functions 

Generated by 🚫 dangerJS against 96aa58f

Base automatically changed from rehan/cdp-feature-modules to main-replica-for-cdp December 1, 2023 14:01
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With independent module initialization, I think we no longer need this

@mrehan27 mrehan27 marked this pull request as ready for review December 1, 2023 18:02
@mrehan27 mrehan27 merged commit b013660 into main-replica-for-cdp Dec 4, 2023
6 of 9 checks passed
@mrehan27 mrehan27 deleted the rehan/cdp-push-module branch December 4, 2023 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants