search

urbanairship / android-library

Urban Airship Android SDK
Other
109 stars 123 forks source link

Airship SDK should not modify global app behaviour by pulling in WorkManagerInitializer #234

Closed bmesing closed 2 months ago

bmesing commented 9 months ago

AirshipSDK comes with an AirshipInitializer that relies on WorkManager. To this end, AirshipInitializer defines a dependency on WorkManagerInitializer. This silently breaks custom work manager implementation as recommended e.g. when using hilt for WorkManager instantiation: https://developer.android.com/training/dependency-injection/hilt-jetpack#workmanager

In https://github.com/urbanairship/android-library/issues/202 you provided a workaround to use a NoDependencyAirshipInitializer. However, I do not consider this an adequate solution. A library (like airship SDK) should never implicitly mess with the global configuration. E.g. for me, when I tried to start using Hilt for WorkManager in our app, I was able to find your workaround only after I figured out the root cause of the problem. It took me several hours to figure out why the proposed mechanism did not work, including debugging into Android system libraries.

Your library breaks seemingly unrelated code with the potential of introducing serious bugs and wasting much time on figuring out what is going on.

Note that I have also created a ticket in the Google bug tracker as well, because I believe part of the problem lies on their end: https://issuetracker.google.com/issues/312025155

rlepinski commented 9 months ago

Thank you for filing a ticket on the google issue tracker. Its frustrating to us as well that its the recommended solution to runnings tasks, but WorkManager exposes APIs that easily break our integration with it.

I agree with you that its not ideal for Airship to break the ability to customize WorkManager. However, with how the startup library works, if we do not define a dependency on WorkManager than we could initialize out of order and our SDK will thrash for no reason. WorkManager does not provide any hook for when its initialized so what our SDK does is queues up the work and retries every second. Not great, but maybe its not the end of the world either.

So our options are: 1) Try to get changes in WorkManager to allow scheduling work before its initialized or at the very least expose a way to listen for when it is initialized 2) Try to see if we can detect when the default WorkManager initializer is removed in the manifest and remove it as a dependency in our initializer 3) Remove dependency and just allow the SDK to do the retry thrash

Going to evaluate option 2 right now to see if that is a happy middle ground while we try to get some changes in WorkManager.

rlepinski commented 9 months ago

Looks like we do not have a way to get the context when returning a list of dependencies so our option is 1 or 3.

bmesing commented 9 months ago

IMHO it should be either force Airship-SDK users to make a conscious decision on wether to use "NoDependencyAirshipInitializer" or regular "AirshipInitializer" (this would still mean the people overriding WorkManagerInitializer after having integrated Airship some month/years before might miss that they need to switch to NoDependencyAirshipInitializer, but at least it might ring a bell) or just work in every setup (i.e. making NoDependencyAirshipInitializer the default).

rlepinski commented 2 months ago

Removed in 18.0.0