launchdarkly / android-client-sdk

LaunchDarkly Client-side SDK for Android
Other
45 stars 23 forks source link

Remove guava as a dependency #16

Closed trevjonez closed 5 years ago

trevjonez commented 7 years ago

From the whole dependency list most are admissible due to common usage on android or based on minimal size impact (dexCount < 2500)

The primary offender being compile 'com.google.guava:guava:19.0'

Based on the very small portion of guava that is actually being consumed it seems a shame to pull in a 2MB jar to only directly use 8 unique classes. The patterns observed here is frequently the reason guava is advocated against on android.

You can see the dependency report on your library here

Actual immediate dependency on guava is as follows:

from com.launchdarkly.android.EventProcessor:

from com.launchdarkly.android.HttpFeatureFlagFetcher:

from com.launchdarkly.android.LDClient:

from com.launchdarkly.android.StreamProcessor:

from com.launchdarkly.android.UserManager:

drichelson commented 7 years ago

Thanks for the feedback and report! Do you have any suggestions for alternatives to our use of Futures?

trevjonez commented 7 years ago

Personally I would use RxJava to handle that type of async / parallel behavior. RxJava is a much more common occurrence on android so it would likely offend less than the dependency on guava. However someone might have the same argument against RX as I have here against guava.

Without requiring some other large library or a paradigm shift, a quicker fix that might bring it down to negligible size would be using jarjar links to produce a repackaged subset of guava. Depending on how large of a subgraph is required this may be much better or hardly any better.

drichelson commented 7 years ago

Thanks for the honest feedback on RxJava vs Guava. I'm not sure using jarjar would work on Guava since we expose a Guava Future on a public interface

We can make the init method return java.util.concurrent.Future instead of Guava's ListenableFuture

condesales commented 7 years ago

ProGuard rules are there for that. If classes are not being used by the library, ProGuard should take care of it and remove then for the final artifact.

trevjonez commented 7 years ago

The provided proguard rules specify: -keep class com.google.common.** { *; }

This is effectively keep all of guava

I can see and even appreciate the proguard argument for keeping guava but it would be nice to at least have a tuned configuration keeping only what is needed.

forrestbice commented 7 years ago

Completely agree with @trevjonez; was pretty shocked by the overall method hit this SDK adds. Really looking forward to improvements though!

Another reason to remove Guava would be that today I just ran into a dependency conflict between OneSignal and Robolectric:

Error:Conflict with dependency 'com.google.guava:guava' in project ':app'. Resolved versions for app (19.0) and test app (20.0) differ. See http://g.co/androidstudio/app-test-app-conflict for details.

I was able to force a resolution via:

resolutionStrategy.force "com.google.guava:guava:20.0"

but this is less than ideal

forrestbice commented 7 years ago

@trevjonez would you mind sharing your Proguard rules for LaunchDarkly? I'm having a lot of trouble with it

trevjonez commented 7 years ago

I ended up adding the following to my project:

-dontwarn com.google.common.**
-dontwarn org.slf4j.**

Really just depending on the magic of proguard to keep the right things. That in mind we have only done very light internal testing with this config and launch darkly included and it has not yet hit the masses yet. Subtle issue may indeed be lurking.

forrestbice commented 7 years ago

@trevjonez hmm are you seeing Proguard complain like in either of the screenshots below? The first was generated when using the LaunchDarkly suggested rules, found here: http://docs.launchdarkly.com/docs/android-sdk-reference/. And the second was those encountered using the rules you mentioned above

screen shot 2017-02-01 at 1 25 38 pm

screen shot 2017-02-01 at 1 28 04 pm

Honestly I'm thinking about using Apptimize or another A/B / Feature Switching SDK that has a smaller footprint and plays more nicely with Proguard. Do you have any suggestions or experience using an alternative? I've wrapped our feature switch logic so that swapping out the web backing service should be pretty quick

trevjonez commented 7 years ago

I do indeed get a pile of Note: items but have not found any issues yet. Although the pro guarded staging release builds only undergo light critical path manual checks, whereas all automated tests are ran against debug non minified builds.

I have personally not looked elsewhere for toggle control as that decision is outside the scope of what I can dictate. I have however considered creating a custom implementation of this library since their rest API has pretty nice documentation. Unfortunately I have not had the spare time to delve into such an endeavor.

EDIT:

The thing breaking your build is the 663 Warning: items.

drichelson commented 7 years ago

Hi, We've just released 2.0.0 which addresses some of the concerns in this thread.

drichelson commented 7 years ago

I'm closing this issue as I think we have addressed the spirit of it. If anyone has further comments or questions please create a support ticket with us: support@launchdarkly.com

lwasylkowski commented 6 years ago

I'd like to reopen this issue, as it's still not fixed -- that you removed Guava class from public API doesn't solve the issue. This library still pulls Guava as transitive dependency, adding over 15k methods and 3.5k fields to the resulting APK. Proper solution would be either to get rid of Guava entirely and use different APIs, or declare Guava as compile-time only dependency (so that Gradle doesn't pull it transitively) and include only required methods, repackaged, as part of the library. Or simply copy necessary classes, if licence permits.

Depending on such big library and saying that consumers should use proguard anyway is not a solution

arun251 commented 6 years ago

Reopening. We'll see what we can reasonably do.

simtel12 commented 5 years ago

@arun251 Any movement on this?

Antimonit commented 5 years ago

Looks like Guava has been removed in the recent release:

https://github.com/launchdarkly/android-client-sdk/releases/tag/2.8.0

gwhelanLD commented 5 years ago

Looks like Guava has been removed in the recent release:

https://github.com/launchdarkly/android-client-sdk/releases/tag/2.8.0

This is correct. We're happy to finally address this suggestion to improve LaunchDarkly's Android SDK. As of the 2.8 release guava is no longer a dependency to the SDK. We appreciate everyone's feedback on this issue and we hope to continue improving all our SDKs taking all input into account.

Thanks!