Refactor singleton service initializers #172
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 8 commits
May 23, 2024 10:46
…t rid of objects, just have to be more particular about using object initializers.
…KlaviyoApiClient being a singleton object that can hit an unrecoverable exception in its initializer...
…a class was complicated by the inner class, and besides it works well as a singleton. The initializer was the real problem, so I extract the listeners from the initializer and created a startService method. This is safe to run multiple times if the SDK is re-initialized.
… body except the methods that are themselves protected anyway
evan-masseau
changed the title
evan-masseau
force-pushed
the
ecm/services-refactor
branch
from
0cd2919
to
31f7479
Compare
evan-masseau
commented
May 28, 2024
sdk/analytics/src/main/java/com/klaviyo/analytics/networking/KlaviyoApiClient.kt
Outdated
Show resolved
Hide resolved
evan-masseau
commented
May 28, 2024
| @@ -61,34 +61,35 @@ internal class KlaviyoTest : BaseTest() { | |||
| } | |||
|
|
|||
| private val capturedProfile = slot<Profile>() | |||
| private val apiClientMock: ApiClient = mockk<ApiClient>().apply { | |||
| private val mockApiClient: ApiClient = mockk<ApiClient>().apply { | |||
There was a problem hiding this comment.
Naming convention fix, sometimes using mock as a prefix, sometimes as a suffix. Suggesting we consolidate to prefix.
evan-masseau
commented
May 28, 2024
| @@ -6,7 +6,26 @@ import com.klaviyo.core.config.Log | |||
| import com.klaviyo.core.config.Log.Level | |||
| import java.util.regex.Pattern | |||
|
|
|||
| open class KLog : Log { | |||
| object KLog : Log { | |||
There was a problem hiding this comment.
KLog doesn't need to be instantiated, it is essentially just a set of static methods anyway.
evan-masseau
commented
May 28, 2024
| @@ -14,10 +14,6 @@ internal object KlaviyoLifecycleMonitor : LifecycleMonitor, Application.Activity | |||
|
|
|||
| private var activityObservers = mutableListOf<ActivityObserver>() | |||
|
|
|||
| init { | |||
| onActivityEvent { Registry.log.verbose(it.type) } | |||
There was a problem hiding this comment.
This one wasn't causing any exceptions, but I thought it best to get rid of any unnecessary object inits to head off the same issues.
…all SDK functions in safeCall, it is more straightforward to log from there.
# Conflicts: # sdk/core/src/main/java/com/klaviyo/core/KlaviyoException.kt
7 tasks
evan-masseau
added
the
major-release
|
Reverted to draft, will need to update once 2.4.0 is release. I think these changes were still valuable. |
added 3 commits
June 11, 2024 10:56
# Conflicts: # README.md # docs/index.html # sdk/analytics/src/main/java/com/klaviyo/analytics/Klaviyo.kt # sdk/analytics/src/main/java/com/klaviyo/analytics/networking/KlaviyoApiClient.kt # sdk/analytics/src/test/java/com/klaviyo/analytics/KlaviyoPreInitializeTest.kt # sdk/analytics/src/test/java/com/klaviyo/analytics/KlaviyoTest.kt # sdk/analytics/src/test/java/com/klaviyo/analytics/KlaviyoUninitializedTest.kt # sdk/analytics/src/test/java/com/klaviyo/analytics/networking/KlaviyoApiClientTest.kt # sdk/analytics/src/test/java/com/klaviyo/analytics/networking/requests/KlaviyoApiRequestTest.kt # versions.properties
# Conflicts: # sdk/analytics/src/main/java/com/klaviyo/analytics/Klaviyo.kt # sdk/analytics/src/test/java/com/klaviyo/analytics/KlaviyoTest.kt # sdk/analytics/src/test/java/com/klaviyo/analytics/KlaviyoUninitializedTest.kt # sdk/analytics/src/test/java/com/klaviyo/analytics/networking/requests/KlaviyoApiRequestTest.kt
evan-masseau
commented
Jun 11, 2024
|
This PR has not seen any updates in the last 16 days. Without further action this PR will be closed in 14 days. To disable further staleness checks add the |
ajaysubra
approved these changes
Sep 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Update: Some of the work originally in this PR wound up in the 2.4 release instead. I've now merged master into rel/3.0 and then merged that into this branch. At this point, this PR is some lower priority changes to some service initializers and a lot of test fixture naming convention corrections.
We had some assumptions about the order in which dependencies are initialized, and encountered unrecoverable exceptions when those assumptions were incorrect. Most importantly, accessing
KlaviyoApiClientbeforeapplicationContextwas available would throw an exception in the initializer. An initialization error cannot be recovered, even ifapplicationContextwere later provided, becauseinitwill only run once.Check List
Changelog / Code Overview
As a safety precaution, I've removed all the object initializers that had an implicit dependency, though the API client was the only one I've observed an issue with from testing with our test apps.
Test Plan
Updated all the unit tests already
Functional changes are smaller now that some of this branch's original work was cherry picked and released in 2.4 after testing.
Related Issues/Tickets
CHNL-8569 -- this does not address all the AC of that ticket.