-
Notifications
You must be signed in to change notification settings - Fork 7
IAF | Implements App Session with Profile, Lifecycle, Company ID observers #265
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
Conversation
Added a bridge message so that webview knows when the local JS asset is present Added a little observer framework for webview to start/stop observers when webview is able to receive
|
||
// Notify the SDK over the native bridge that these local JS scripts are initialized | ||
var bridgeName = document.head.getAttribute("data-native-bridge-name") || "" | ||
|
||
if (window[bridgeName] && window[bridgeName].postMessage) { | ||
window[bridgeName].postMessage(JSON.stringify({type: "jsReady"})) | ||
} else { | ||
console.error("Unknown bridge name: " + bridgeName) | ||
} |
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.
Had to roll my own sorta lifecycle event to know when this JS is loaded.
sdk/forms/src/main/java/com/klaviyo/forms/bridge/BridgeMessage.kt
Outdated
Show resolved
Hide resolved
sdk/forms/src/main/java/com/klaviyo/forms/bridge/BridgeMessage.kt
Outdated
Show resolved
Hide resolved
sdk/forms/src/main/java/com/klaviyo/forms/webview/KlaviyoWebViewClient.kt
Outdated
Show resolved
Hide resolved
sdk/forms/src/main/java/com/klaviyo/forms/webview/KlaviyoWebViewClient.kt
Show resolved
Hide resolved
sdk/forms/src/main/java/com/klaviyo/forms/webview/KlaviyoWebViewClient.kt
Show resolved
Hide resolved
sdk/forms/src/test/java/com/klaviyo/forms/bridge/KlaviyoBridgeMessageHandlerTest.kt
Outdated
Show resolved
Hide resolved
sdk/forms/src/main/java/com/klaviyo/forms/bridge/BridgeMessage.kt
Outdated
Show resolved
Hide resolved
sdk/forms/src/main/java/com/klaviyo/forms/bridge/BridgeMessage.kt
Outdated
Show resolved
Hide resolved
sdk/forms/src/main/java/com/klaviyo/forms/bridge/BridgeMessage.kt
Outdated
Show resolved
Hide resolved
sdk/forms/src/main/java/com/klaviyo/forms/bridge/BridgeMessage.kt
Outdated
Show resolved
Hide resolved
sdk/forms/src/main/java/com/klaviyo/forms/bridge/BridgeMessage.kt
Outdated
Show resolved
Hide resolved
sdk/forms/src/main/java/com/klaviyo/forms/bridge/ObserverCollection.kt
Outdated
Show resolved
Hide resolved
sdk/forms/src/main/java/com/klaviyo/forms/bridge/ObserverCollection.kt
Outdated
Show resolved
Hide resolved
sdk/forms/src/main/java/com/klaviyo/forms/webview/KlaviyoWebViewClient.kt
Outdated
Show resolved
Hide resolved
sdk/forms/src/main/java/com/klaviyo/forms/webview/KlaviyoWebViewClient.kt
Outdated
Show resolved
Hide resolved
sdk/forms/src/main/java/com/klaviyo/forms/webview/KlaviyoWebViewClient.kt
Outdated
Show resolved
Hide resolved
#267) * Implements lifecycle observers to support foreground/backgrounding Added a foregrounding event to lifecycle monitor * Remove all the session behavior references. * PR Feedback --------- Co-authored-by: Evan Masseau <>
* Add profile observer implementation * PR Feedback -- readability, lazy load, keep identifier keys in analytics package --------- Co-authored-by: Evan Masseau <>
I am a man of my word. Co-authored-by: Evan Masseau <>
* Fixes to state observers: - We can't re-instantiate State every time initialize is called: now that another module relies on State observers, we must be able to keep those observers in memory. Luckily, there's no ill effect of treating State as a singleton (in fact, we used to have it as an Object instead of Class) - We need state observers to be able to start/stop in reaction to a state change, so our prior concurrency solution is insufficient because that will encounter a concurrent mutation error. CopyOnWrite is the easy solution (converting to Rx is the better long term, but more complex?) * Convert the callback type for state observers from a two-arg key/value thing, which was confusing, to a strongly typed data class, left the old one in so this isn't a breaking change, even if it is just core * Take out registerOnce methods that take an instance, too easy to mix up. * Cover all our existing observers with the copy on write fix. --------- Co-authored-by: Evan Masseau <>
* Move thread util to core package so it can be shared * Keep the webview in memory when detaching (i.e. after closing) * Add company ID observer to re-initialize the webview when company changes * Add unregister and reinitialize methods * Config validation --------- Co-authored-by: Evan Masseau <>
init { | ||
/** | ||
* Since the analytics module owns ApiClient, we must register it. | ||
* | ||
* This registration is a lambda invoked when the API service is required. | ||
* KlaviyoApiClient service is not being initialized here. | ||
*/ | ||
Registry.registerOnce<ApiClient> { KlaviyoApiClient } | ||
/** | ||
* Since the analytics module owns these services, it must register them. | ||
* | ||
* This registration is a lambda invoked when the service is required, not instantiated now | ||
*/ | ||
private fun initializeServices() = Registry.apply { | ||
registerOnce<ApiClient> { KlaviyoApiClient } | ||
registerOnce<State> { | ||
KlaviyoState().also { state -> | ||
register<StateSideEffects>(StateSideEffects(state)) | ||
} | ||
} | ||
} |
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.
Previously I was using the one-time nature of an init
(called the first time you access Klaviyo, usually to initialize) to have APIClient register only once. Now that we have RegisterOnce
, I don't have to use an Object initializer, which I tend to avoid anyway.
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.
Feels scary to approve something so big but we did go about it in a pretty methodical review manner. Nice work!
@@ -83,10 +89,6 @@ object Klaviyo { | |||
|
|||
Registry.get<ApiClient>().startService() | |||
|
|||
Registry.register<State>(KlaviyoState()) | |||
Registry.getOrNull<StateSideEffects>()?.detach() |
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.
just to confirm we don't want to detach anymore? Or did this functionality change
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.
Detaching here was an artifact of creating a new State instance. Under the hood, state is actually unchanged, since by creating a new instance it was really just re-loading state from disk, but all properties in state are automatically backed up to disk on set anyways. So now that I'm not re-starting state, I don't need to restart these observers either.
…rvers (#265) * Added a bridge message so that webview knows when the local JS asset is present * Added a little observer framework for webview to start/stop observers when webview is able to receive * IAF | Lifecycle observers to support foreground/backgrounding tracking (#267) * Implements lifecycle observers to support foreground/backgrounding. Added a foregrounding event to lifecycle monitor * IAF | Profile observers (#266) * Adjusted several class/interface names in the bridge subpackage (#268) * State management/observer improvements (#271) * Fixes to state observers: - We can't re-instantiate State every time initialize is called: now that another module relies on State observers, we must be able to keep those observers in memory. Luckily, there's no ill effect of treating State as a singleton (in fact, we used to have it as an Object instead of Class) - We need state observers to be able to start/stop in reaction to a state change, so our prior concurrency solution is insufficient because that will encounter a concurrent mutation error. CopyOnWrite is the easy solution (converting to Rx is the better long term, but more complex?) * Convert the callback type for state observers from a two-arg key/value thing, which was confusing, to a strongly typed data class, left the old one in so this isn't a breaking change, even if it is just core * IAF | Persisting the webview after close (#270) * Move thread util to core package so it can be shared * Keep the webview in memory when detaching (i.e. after closing) * Add company ID observer to re-initialize the webview when company changes * Add unregister and reinitialize forms API methods
Description
This builds on the previous PR #264 to start observing events (e.g. lifecycle events and profile state changes) to be injected into the WebView. This adds the interfaces and organizational code for those observers. To try to break things up a bit, I will have subsequent PRs to add observers for lifecycle events, profile changes, and company ID change individually.
Due Diligence
Release/Versioning Considerations
Patch
Contains internal changes or backwards-compatible bug fixes.Minor
Contains changes to the public API.Major
Contains breaking changes.Changelog / Code Overview
jsReady
to deal with a timing issue. The JS isn't immediately ready when we load in the HTML string, or even in theonPageStarted
callback method. But waiting for the handshake is too late to inject the initial profile data.Observer
interface for individual observer types (e.g profile data, analytics events, lifecycle)ObserverCollection
interface to collect and manage all observersHandshakeSpec
data class rather than codingJSONObjects
directly. This way I can assign handshake type/version in theObserver
instances, where the logic actually lives.Plus, these branches were built on top of this, and merged in:
Test Plan
The base work in this PR was organizational, and sufficiently covered by unit tests and a basic regression cycle.
Each PR built on top of it had its own testing steps that I'm going to copy over to be tested as an integrated branch:
Segmentation:
To your klaviyo test account, add a web form that is targeted to a segment/list. Note an email or other identifier of a profile in that segment that should see the form. You can use the iaf-tester.html to confirm this web form does appear to that profile.
Build android test app with composite build pointing at this branch of the SDK.
In the test app, input the profile identifier from above. On the forms page, change form environment to web.
Trigger a form -- you should see your targeted form!
App Session
registerForInAppForms
with the session timeout set to something low like 10 seconds.Unregister method
To test the new unregister method, add a button to the test app UI that calls unregister.
Misc
State change observer implementation changed a bit under the hood. To verify it is still working:
Make an API call with an invalid identifier -- StateSideEffects is responsible for catching the 400 error and removing that value from state so you should see this value removed from the test app UI
Re-initialize with a different company ID -- you should see an API call to unregister push token
Repeat step 1 -- should get the same result as before
Related Issues/Tickets
CHNL-19959