-
Notifications
You must be signed in to change notification settings - Fork 376
feat: Add Kotlin MainApplication and suspend initialization support #2374
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
base: 5.4.0-alpha-01-base
Are you sure you want to change the base?
Conversation
- Add MainApplicationKT.kt as Kotlin version of MainApplication.java - Add initWithContextSuspend() method for async initialization - Refactor OneSignalImp to use IO dispatcher internally for initialization - Add comprehensive unit tests for suspend initialization - Rename LatchAwaiter to CompletionAwaiter for better semantics - Add helper classes for user management (AppIdHelper, LoginHelper, LogoutHelper, UserSwitcher) - Update build.gradle to include Kotlin coroutines dependency - Ensure ANR prevention by using background threads for initialization
@OptIn(DelicateCoroutinesApi::class) | ||
fun scheduleStart() { | ||
Thread { | ||
GlobalScope.launch(Dispatchers.Default) { |
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.
Creating a thread is very expensive. Google promotes usages of Dispatcher
https://kotlinlang.org/docs/coroutines-basics.html#coroutines-are-light-weight
}.build() | ||
|
||
// get the current config model, if there is one | ||
private val configModel: ConfigModel by lazy { services.getService<ConfigModelStore>().model } |
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.
earlier it was a var and we could probably run into race conditions.
lazy is thread safe and handles concurrent access automatically.
and most importantly its null safe and we dont need to use !!
anymore.
private val logoutLock: Any = Any() | ||
private val userSwitcher by lazy { | ||
val appContext = services.getService<IApplicationService>().appContext | ||
UserSwitcher( |
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.
the class is broken down significantly into all these subclasses to address seperation of concern and improve testability
jwtBearerToken: String?, | ||
) { | ||
Logging.log(LogLevel.DEBUG, "login(externalId: $externalId, jwtBearerToken: $jwtBearerToken)") | ||
Logging.log(LogLevel.DEBUG, "Calling deprecated login(externalId: $externalId, jwtBearerToken: $jwtBearerToken)") |
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.
Logging deprecate calls so that someday we can get an estimate its usage and plan on its removal.
if (AndroidUtils.isRunningOnMainThread()) { | ||
Logging.warn("This is called on main thread. This is not recommended.") | ||
} | ||
} catch (e: RuntimeException) { |
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.
The exception catch here is for mostly testing
*/ | ||
suspend fun login( | ||
context: Context, | ||
appId: String, |
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.
I see a bunch of crashes where login is called before initializing the SDK.
Is it ok to just accept the appId and context here and initialize the SDK (if its not done already) and continuing with the login process? It shouldn't be very hard for the consumer to pass the AppId IMHO.
Does it impact anything else?
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.
Thinking about this as a consumer of this SDK, having both initialize
and login
with appId
raises questions;
- Does it matter if which one I pass appId to?
- Is one better than the other?, which one is preferred?
- Does this means login is designed to move the User to a different OneSignal app?
As far as internals of the SDK goes, it doesn't matter, but you see the confusion this would add to the public API.
*/ | ||
suspend fun logout( | ||
context: Context, | ||
appId: String, |
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.
same with logout.
@Deprecated(message = "Use suspend version", ReplaceWith("suspend fun logout()")) | ||
fun logout() |
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.
There isn't a return value, so it is valid to call it and not wait. What do you think about keeping both for these cases?
Description
One Line Summary
Modernizing the SDK to use Kotlin Coroutines
Details
Motivation
Convert MainApplication from Java to Kotlin with modern async handling using coroutines, add suspend methods to OneSignal SDK for better ANR prevention, and deprecate the Java implementation to encourage migration to the improved Kotlin version.
Scope
SDK Initialization
Added new public APIs
Testing
Unit testing
Added a bunch of tests
Manual testing
Tested happy path
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is