Attach lifecycle listeners internally in initialize method #131
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
Mark the public property to be deprecated and replace with a no-op implementation to avoid duplicate listeners. Update readme instructions, migration guide.
evan-masseau
changed the title
| * and prevent duplicate registration of the KlaviyoLifecycleMonitor | ||
| * until next major release when we can make the breaking change to remove that public property | ||
| */ | ||
| object NoOpLifecycleCallbacks : Application.ActivityLifecycleCallbacks { |
There was a problem hiding this comment.
So this would also be removed in next major release once we no longer need to worry about duplicate lifecycle registration?
There was a problem hiding this comment.
Correct, I just can't remove it in a minor release by semantic versioning rules
jordan-griffin
approved these changes
Feb 16, 2024
ndurell
reviewed
Feb 16, 2024
| ```kotlin | ||
| registerActivityLifecycleCallbacks(Klaviyo.lifecycleCallbacks) | ||
| ``` | ||
| For version 2.1.x, `Klaviyo.lifecycleCallbacks` has been replaced with a no-op implementation to avoid duplicative |
ndurell
reviewed
Feb 16, 2024
| application?.apply { | ||
| unregisterActivityLifecycleCallbacks(Registry.lifecycleCallbacks) | ||
| registerActivityLifecycleCallbacks(Registry.lifecycleCallbacks) | ||
| } ?: throw LifecycleException() |
There was a problem hiding this comment.
Would this crash their app? Should it log a warning instead?
ndurell
approved these changes
Feb 16, 2024
evan-masseau
commented
Feb 16, 2024
| @@ -47,6 +52,12 @@ object Klaviyo { | |||
| .applicationContext(applicationContext) | |||
| .build() | |||
| ) | |||
|
|
|||
| val application = applicationContext.applicationContext as? Application | |||
There was a problem hiding this comment.
Here is the documentation on applicationContext
In earlier speccing last year, I shied away from using this method to access the Application object because of a stackoverflow post from 2011 that warned off using it. Now that I've read more about it, I think that post was incorrect. For example, this description from official docs describes exactly what we're doing here:
Return the context of the single, global Application object of the current process. This generally should only be used if you need a Context whose lifecycle is separate from the current context, that is tied to the lifetime of the process rather than the current component.
kennyklaviyo
approved these changes
Feb 20, 2024
added 2 commits
February 20, 2024 16:11
…ecm/lifecycle # Conflicts: # sdk/analytics/src/main/java/com/klaviyo/analytics/Klaviyo.kt
evan-masseau
added a commit
that referenced
this pull request
Feb 21, 2024
…ze method (#131)* Internalize the lifecycle listener attachment.* Mark the public property to be deprecated and replace with a no-op implementation to avoid duplicate listeners.* Update readme instructions, migration guide.---------Co-authored-by: Evan Masseau <>" (#136) * Auto generate documentation PRs (#124) Automate all the things Co-authored-by: Evan Masseau <> * Minor README and Documentation Updates (#121) * Fixed SDK name in license section * Fixed appearance of some of the advanced section after seeing it on the dokka html * Other grammar/spelling fixes --------- Co-authored-by: Evan Masseau <> * Add note about anonymous profiles to readme (#127) --------- Co-authored-by: Evan Masseau <> * Wrap public methods in try/catch (#129) Added safety checks around public API methods to prevent throwing exceptions that would otherwise crash the host application --------- Co-authored-by: Evan Masseau <> * Logging (#130) - Enable logging in published SDK - Added level-aware logging with a configurable threshold from code or manifest file. - Added warning level - Threshold has a default based on debug/release, can be configured from code and from manifest - Updated tests, removed some old log related test code from earlier build variants/flavors - Adjusted log levels throughout the code --------- Co-authored-by: Evan Masseau <> * Attach lifecycle listeners internally in initialize method (#131) * Internalize the lifecycle listener attachment. * Mark the public property to be deprecated and replace with a no-op implementation to avoid duplicate listeners. * Update readme instructions, migration guide. --------- Co-authored-by: Evan Masseau <> * Generated docs for e9598b0 --------- Co-authored-by: Evan C Masseau <5167687+evan-masseau@users.noreply.github.com> Co-authored-by: github-actions <github-actions@github.com>
evan-masseau
pushed a commit
that referenced
this pull request
Mar 18, 2024
…ation * commit 'b12ef65b0d460b343c4d1eb2bac6e227a6f51d62': Release Version 2.1.0 (#137) Attach lifecycle listeners internally in initialize method (#131) Logging (#130) Wrap public methods in try/catch (#129) Add note about anonymous profiles to readme (#127) Minor README and Documentation Updates (#121) Auto generate documentation PRs (#124)
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
While working on a ticket to automatically attach android lifecycle listeners for react native SDK, I've realized we could extend that convenience to the android SDK.
Check List
lifecycleCallbacksdeprecatedChangelog / Code Overview
initializemethod instead of requiring app developers to do it.Test Plan
Related Issues/Tickets
CHNL-4635