refactor!: singleton API only way to use SDK now #209

levibostian · 2022-11-04T03:45:00Z

QA

This PR touches almost the entire codebase and during my own QA testing, I have found a few bugs. So, a full QA walk through with the RH app is recommended.

Things to test in particular:

Install build of RH app before pr.78 build. Run through the app a little bit to make sure some tasks are added to the background queue and profile and device token have been saved to app's data storage. Then, install pr.78 build of RH. Open app and make sure that the previously identified profile, device token, and background queue tasks all run as expected. This tests that customers upgrading to this version of SDK will not lose data going from SDK v2 to v3.
Open app, do some tasks, force quit app, open it up again. The SDK initialization code has been changed so we want to make sure that it's working correctly when the RH opens and it initializes the SDK.
Push notifications and in-app messages are received.
Profile and device attributes get added successfully to Fly. RH app adds these automatically. We need to confirm that Fly receives them.

Closes: https://github.com/customerio/issues/issues/6933

This PR removes the ability to use the iOS SDK in a non-singleton way. You used to be able to create 1+ instances of the SDK: CustomerIO() but now you have to use CustomerIO.shared.

Notes:

This PR will trigger a breaking change to the SDK and will trigger a major version bump to v3.0
This is a bigger PR since it's a larger refactor. I made comments on parts of the code I find interesting in the code review.

codecov · 2022-11-04T03:50:14Z

Codecov Report

Merging #209 (dec4535) into develop (b8ba9b6) will decrease coverage by 1.54%.
The diff coverage is 44.16%.

@@             Coverage Diff             @@
##           develop     #209      +/-   ##
===========================================
- Coverage    52.30%   50.76%   -1.55%     
===========================================
  Files          101      102       +1     
  Lines         1172     1113      -59     
===========================================
- Hits           613      565      -48     
+ Misses         559      548      -11

Impacted Files	Coverage Δ
Sources/Common/Background Queue/Queue.swift	`64.28% <ø> (ø)`
Sources/Common/Background Queue/QueueStorage.swift	`93.33% <ø> (ø)`
...es/Common/Extensions/UIApplicationExtensions.swift	`0.00% <ø> (ø)`
Sources/Common/Service/HttpClient.swift	`92.85% <ø> (ø)`
Sources/Common/Type/Region.swift	`100.00% <ø> (ø)`
...es/Common/autogenerated/AutoLenses.generated.swift	`13.15% <ø> (+1.79%)`	⬆️
...ogenerated/AutoDependencyInjection.generated.swift	`40.00% <0.00%> (+20.00%)`	⬆️
...rces/MessagingPush/MessagingPush+AppDelegate.swift	`0.00% <ø> (ø)`
...essagingPush/RichPush/MessagingPush+RichPush.swift	`0.00% <ø> (ø)`
...essagingPush/RichPush/RichPushRequestHandler.swift	`0.00% <ø> (ø)`
... and 49 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

levibostian · 2022-11-04T03:52:39Z

Sources/Common/Store/GlobalDataStore.swift

-    func appendSiteId(_ siteId: String)
-    // all site ids that have ever been registered with the SDK
-    var siteIds: [String] { get }


This code was originally added to the SDK so we could do something such as running background queue tasks for all of the site-ids ever registered to the SDK. Today, we only run background queue tasks for the currently initialized site-id (most to all of our customers use our SDK in this way).

However, we have never used this feature so I removed it. We can bring it back in the future if we decide we need this feature.

levibostian · 2022-11-04T03:54:51Z

Sources/Common/Store/SdkConfig.swift

-public protocol SdkConfigStore: AutoMockable {
-    var config: SdkConfig { get set }
-}
-
-// sourcery: InjectRegister = "SdkConfigStore"
-// sourcery: InjectSingleton
-public class InMemorySdkConfigStore: SdkConfigStore {
-    @Atomic public var config = SdkConfig()
-}


No more need for a SDK store. We now store the SDKConfig instance in the dependency injection graph.

levibostian · 2022-11-04T04:08:14Z

Sources/MessagingInApp/MessagingInAppImplementation.swift

+import Foundation
+import Gist
+
+internal class MessagingInAppImplementation: MessagingInAppInstance {


All of this logic was copy/pasted from MessagingInApp class. To follow the same pattern that CustomerIO and MessagingPush classes follow with an implementation object.

levibostian · 2022-11-04T04:16:06Z

Sources/MessagingPush/RichPush/RichPushRequestHandler.swift

+        let sdkInitializedUtil = SdkInitializedUtilImpl()
+
+        guard let postSdkInitializedData = sdkInitializedUtil.postInitializedData else { return }
+
+        let diGraph = postSdkInitializedData.diGraph


This is the new (safe) way to get the DiGraph instance. You see other files in this PR also doing it this way.

README.md

levibostian · 2022-11-04T14:23:55Z

Sources/Common/Util/KeyValueStorage.swift

+            siteIdPart = ".\(siteId)"
+        }
+
+        return "io.customer.sdk\(appUniqueIdentifier)\(siteIdPart)"


Reminder to self: QA test this to make sure that we are not changing the name after customer upgrades. To avoid customers losing data.

ami-oss-ci · 2022-11-04T14:28:28Z

Pull request title looks good 👍!

If this pull request gets merged, it will cause a new release of the software. Example: If this project's latest release version is 1.0.0. If this pull request gets merged in, the next release of this project will be 2.0.0. This pull request is a breaking change.

All merged pull requests will eventually get deployed. But some types of pull requests will trigger a deployment (such as features and bug fixes) while some pull requests will wait to get deployed until a later time.

To merge this pull request, add the label Ready to merge to this pull request and I'll merge it for you.

This project uses a special format for pull requests titles. Expand this section to learn more (expand by clicking the ᐅ symbol on the left side of this sentence)...

This project uses a special format for pull requests titles. Don't worry, it's easy!

This pull request title should be in this format:

<type>: short description of change being made

If your pull request introduces breaking changes to the code, use this format:

<type>!: short description of breaking change

where <type> is one of the following:

feat: - A feature is being added or modified by this pull request. Use this if you made any changes to any of the features of the project.
fix: - A bug is being fixed by this pull request. Use this if you made any fixes to bugs in the project.
docs: - This pull request is making documentation changes, only.
refactor: - A change was made that doesn't fix a bug or add a feature.
test: - Adds missing tests or fixes broken tests.
style: - Changes that do not effect the code (whitespace, linting, formatting, semi-colons, etc)
perf: - Changes improve performance of the code.
build: - Changes to the build system (maven, npm, gulp, etc)
ci: - Changes to the CI build system (Travis, GitHub Actions, Circle, etc)
chore: - Other changes to project that don't modify source code or test files.
revert: - Reverts a previous commit that was made.

Examples:

feat: edit profile photo
refactor!: remove deprecated v1 endpoints
build: update npm dependencies
style: run formatter

Need more examples? Want to learn more about this format? Check out the official docs.

Note: If your pull request does multiple things such as adding a feature and makes changes to the CI server and fixes some bugs then you might want to consider splitting this pull request up into multiple smaller pull requests.

ami-oss-ci · 2022-11-04T14:28:54Z

	Warnings
⚠️	I noticed file `Tests/Tracking/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`
⚠️	I noticed file `Tests/MessagingPushFCM/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`
⚠️	I noticed file `Tests/MessagingPushAPN/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 b31ecbd

jatinn

could use review from another engineer more familiar with swift.
overall looks fine main thing that feels odd to me is we pass both site id & di graph into many initialize calls which feels redundant as some dont use site id and since it seems like can get site id from the di graph.

README.md

Sources/Common/Store/GlobalDataStore.swift

jatinn · 2022-11-08T22:56:11Z

Sources/Common/Store/SdkConfig.swift

    /**
     Base URL to use for the Customer.io track API. You will more then likely not modify this value.

     If you override this value, `Region` set when initializing the SDK will be ignored.
     */
-    public var trackingApiUrl: String = ""
+    public var trackingApiUrl: String


afaik swift cares about initialized & uninitialized variables. so before it was initialized to an empty string as a default, now that it is not any edge cases to consider if people dont set region?

Not sure what you mean by, "...if people don't set region".

Having the default value be "" is error-prone, IMO. Our SDK should set a value for this when the SDK is initialized. So, by me removing the default, it reminds us to set a value for it at SDK initialization.

Sources/Common/Util/KeyValueStorage.swift

Sources/MessagingInApp/MessagingInAppImplementation.swift

jatinn · 2022-11-11T21:31:46Z

Sources/MessagingInApp/MessagingInAppImplementation.swift

+
+    private func getDeliveryId(from message: Message) -> String? {
+        guard let deliveryId = message.gistProperties.campaignId else {
+            logger


not sure if we want this as an error message as test delivery sends do not have delivery id set

Yeah, I would agree to not use an error log. Error logs are logs that we hope customers see and then report to us.

....knowing that delivery id is not set on test sends, I should probably make deliveryId optional for in-app event listener PR I made?

Sources/MessagingPush/MessagingPush.swift

jatinn · 2022-11-11T21:51:52Z

Tests/Common/Util/KeyValueStorageTest.swift

+            siteId: testSiteId,
+            deviceMetricsGrabber: deviceMetricsGrabberMock
+        )
+        newInstance.switchToGlobalDataStore()


this just clears siteId so isnt this the same as UserDefaultsKeyValueStorage(deviceMetricsGrabber: deviceMetricsGrabberMock)

There are 2 required parameters of UserDefaultsKeyValueStorage constructor. So, I do not believe UserDefaultsKeyValueStorage(deviceMetricsGrabber: deviceMetricsGrabberMock) is possible.

Tests/Shared/UnitTest.swift

levibostian · 2022-11-16T05:10:28Z

@matt-frizzell came up with an idea of us waiting to deploy this change until this feature is merged as well.

ami-oss-ci · 2022-11-17T17:11:33Z

	Warnings
⚠️	I noticed file `Tests/MessagingPushAPN/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`
⚠️	I noticed file `Tests/MessagingPushFCM/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`
⚠️	I noticed file `Tests/Tracking/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 c208835

ami-oss-ci · 2022-11-17T17:14:54Z

	Warnings
⚠️	I noticed file `Tests/MessagingPushAPN/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`
⚠️	I noticed file `Tests/MessagingPushFCM/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`
⚠️	I noticed file `Tests/Tracking/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 dec4535

## [2.0.0-alpha.1](1.2.6...2.0.0-alpha.1) (2022-11-30) ### ⚠ BREAKING CHANGES * make delivered push metric more reliable * remove FCM dependency from cocoapods (#210) * singleton API only way to use SDK now (#209) ### Bug Fixes * fix compile time errors notification service extensions ([#214](#214)) ([bd5911b](bd5911b)) * make delivered push metric more reliable ([0478e52](0478e52)) ### Code Refactoring * remove FCM dependency from cocoapods ([#210](#210)) ([3547076](3547076)) * singleton API only way to use SDK now ([#209](#209)) ([72b7477](72b7477))

ami-ci · 2022-11-30T17:34:00Z

🎉 This PR is included in version 2.0.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

## [2.0.0-beta.1](1.2.7...2.0.0-beta.1) (2022-12-09) ### ⚠ BREAKING CHANGES * make delivered push metric more reliable * remove FCM dependency from cocoapods (#210) * singleton API only way to use SDK now (#209) ### Bug Fixes * add sdkwrapperconfig to rich push SDK config ([#226](#226)) ([e43b4cf](e43b4cf)) * do not modify custom attributes casing ([#234](#234)) ([8160fdf](8160fdf)) * fix compile time errors notification service extensions ([#214](#214)) ([bd5911b](bd5911b)) * make delivered push metric more reliable ([0478e52](0478e52)) * sdk not able to compile in ios app ([#225](#225)) ([e4d1b3f](e4d1b3f)) ### Code Refactoring * remove FCM dependency from cocoapods ([#210](#210)) ([3547076](3547076)) * singleton API only way to use SDK now ([#209](#209)) ([72b7477](72b7477))

## [2.0.0](1.2.7...2.0.0) (2022-12-13) ### ⚠ BREAKING CHANGES * make delivered push metric more reliable * remove FCM dependency from cocoapods (#210) * singleton API only way to use SDK now (#209) ### Bug Fixes * add sdkwrapperconfig to rich push SDK config ([#226](#226)) ([e43b4cf](e43b4cf)) * do not modify custom attributes casing ([#234](#234)) ([8160fdf](8160fdf)) * fix compile time errors notification service extensions ([#214](#214)) ([bd5911b](bd5911b)) * make delivered push metric more reliable ([0478e52](0478e52)) * sdk not able to compile in ios app ([#225](#225)) ([e4d1b3f](e4d1b3f)) ### Code Refactoring * remove FCM dependency from cocoapods ([#210](#210)) ([3547076](3547076)) * singleton API only way to use SDK now ([#209](#209)) ([72b7477](72b7477))

levibostian added 5 commits October 28, 2022 14:27

remove constructors for public classes

1409add

refactor: simplify digraph only 1 instance in memory at a time

8144244

remove config from rich push

50e4dcc

finish most TODOs

8f539da

fix tests

bba0714

levibostian commented Nov 4, 2022

View reviewed changes

levibostian added 2 commits November 4, 2022 09:14

changes after self code review

0d018a2

make lint happy

b56a494

levibostian commented Nov 4, 2022

View reviewed changes

README.md Show resolved Hide resolved

levibostian commented Nov 4, 2022

View reviewed changes

levibostian added 2 commits November 4, 2022 09:26

update readme

d9a5b6b

Merge branch 'develop' into levi/remove-multiple-sdk-instances

ce0de69

fix build

74586d8

levibostian requested review from jatinn and ami-aman November 4, 2022 14:59

levibostian self-assigned this Nov 4, 2022

levibostian added 2 commits November 7, 2022 00:04

fix: fixed a bug found during QA testing

eae9c63

fixed bug in app module not setting up during initialize

b31ecbd

levibostian added will need qa QA-Ready labels Nov 7, 2022

levibostian mentioned this pull request Nov 8, 2022

feat: SDK v2 customerio/RemoteHabits-iOS#78

Merged

jasonto added QA-CurrentlyTesting and removed QA-Ready labels Nov 10, 2022

jatinn approved these changes Nov 11, 2022

View reviewed changes

levibostian added 2 commits November 15, 2022 22:55

make changes from pr review

68b4806

remove comment [skip ci]

12ca20b

levibostian mentioned this pull request Nov 16, 2022

feat: add in-app event listener #211

Merged

4 tasks

jasonto added QA-Passed and removed QA-CurrentlyTesting labels Nov 16, 2022

Merge branch 'develop' into levi/remove-multiple-sdk-instances

c208835

fix some code linter changed

dec4535

levibostian merged commit 72b7477 into develop Nov 17, 2022

levibostian deleted the levi/remove-multiple-sdk-instances branch November 17, 2022 18:01

ami-ci added the released on @alpha label Nov 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: singleton API only way to use SDK now #209

refactor!: singleton API only way to use SDK now #209

levibostian commented Nov 4, 2022 •

edited

codecov bot commented Nov 4, 2022 •

edited

levibostian Nov 4, 2022 •

edited

levibostian Nov 4, 2022

levibostian Nov 4, 2022

levibostian Nov 4, 2022

levibostian Nov 4, 2022

ami-oss-ci commented Nov 4, 2022 •

edited

Examples:

ami-oss-ci commented Nov 4, 2022 •

edited

jatinn left a comment

jatinn Nov 8, 2022

levibostian Nov 16, 2022

jatinn Nov 11, 2022

levibostian Nov 16, 2022 •

edited

jatinn Nov 11, 2022

levibostian Nov 16, 2022

levibostian commented Nov 16, 2022

ami-oss-ci commented Nov 17, 2022

ami-oss-ci commented Nov 17, 2022

ami-ci commented Nov 30, 2022

refactor!: singleton API only way to use SDK now #209

refactor!: singleton API only way to use SDK now #209

Conversation

levibostian commented Nov 4, 2022 • edited

QA

codecov bot commented Nov 4, 2022 • edited

Codecov Report

levibostian Nov 4, 2022 • edited

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

ami-oss-ci commented Nov 4, 2022 • edited

Examples:

ami-oss-ci commented Nov 4, 2022 • edited

jatinn left a 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

levibostian Nov 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levibostian commented Nov 16, 2022

ami-oss-ci commented Nov 17, 2022

ami-oss-ci commented Nov 17, 2022

ami-ci commented Nov 30, 2022

levibostian commented Nov 4, 2022 •

edited

codecov bot commented Nov 4, 2022 •

edited

levibostian Nov 4, 2022 •

edited

ami-oss-ci commented Nov 4, 2022 •

edited

ami-oss-ci commented Nov 4, 2022 •

edited

levibostian Nov 16, 2022 •

edited