-
Notifications
You must be signed in to change notification settings - Fork 27
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
Optimize and fix android dependencies #299
Optimize and fix android dependencies #299
Conversation
- set the Android theme colors to match the compose theme colors on app startup.
- remove Koin-Android dependency which is not needed when adding Koin-Androidx-Compose dependency.
…support kotlinx.datetime on API < 26)
…optimizations) and remove unnecessary rules
…with Compose Multiplatform 1.6.2
… on Kotlin 2.0.0 (the project depends on Kotlin 1.9.23)
Wow, nice! Thanks for this ❤️ ! I'll definitely need to take a deeper look but this looks great 👍 . TIL R8 could not tree shake the resources! Initial comments:
I think this is fine?
LGTM to remove them. We had plans to introduce some features but never got to it so I'm down for removing the dependency, We can alsways reintroduce later. Ping @enthuan? Ok for you? |
I didn't notice any issue, I just found suspicious that the entire app is using a beta version of the standard library that didn't match the compiler version, especially with the multiplatform stuff. If you think it's safe then I can revert the change. |
I'm considering making a talk about limitations and workarounds of code and resources shrinking. |
Right, native could be an issue (JVM has always proven to work quite well in the past). JS doesn't work for an example. If anything, I'll probably update this repo to use
Looking forward to it :) |
@@ -38,7 +38,7 @@ class SigningConfigPlugin : Plugin<Project> { | |||
isMinifyEnabled = true | |||
isShrinkResources = true | |||
proguardFiles( | |||
getDefaultProguardFile("proguard-android.txt"), |
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.
Wouldn't it be better to use proguard-android-optimized here to benefit from default rules with all possible optimizations?
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.
These rules are better but still way too conservative in my opinion, and they haven't been updated for ages. I can give you a detailed explanation later.
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 am aware that they are outdated. My thought was it might be still safer for a small project.
I totally agree that adding rules based on your project is better.
@@ -35,6 +35,11 @@ kotlin { | |||
} | |||
} | |||
|
|||
configurations.configureEach { | |||
// Remove unnecessary dependency of Precompose and Moko |
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.
Should we file a bug there?
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.
Yes, as far as I can tell these libraries depend on Core, Activity, or Lifecycle but not on AppCompat directly.
It's an old habit to just add AppCompat as a dependency with all the dependencies it brings along but if the code doesn't use Views and doesn't directly depend on any code in the package androidx.appcompat, it's a mistake.
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.
…nts for Wear release builds
…mpose 1.3.x and Compose 1.6.4 currently used in the project
I focused on the Wear app and applied similar optimizations. I noticed a mismatch of Compose versions caused by Horologist, plus a bogus dependency from Horologist to both Material Components and AppCompat (!) and fixed these. |
AppCompat is everywhere! |
Follow up PR to bump Kotlin to 2.0.0-RC1: #304 |
I created a pull request to fix the dependencies of Horologist 0.5.x: google/horologist#2215 |
This pull request optimizes and fixes the Android and Wear app dependencies and build configuration, with the following effects:
java.time
APIs (Java API desugaring must be enabled to be able to use kotlinx.datetime for older Android versions).Detailed changes:
@Serializable
classes), Compose Material (only Compose Material3 is used), Firebase Installations, Firebase Analytics, Navigation Compose for Android (Wear OS uses Wear Navigation Compose, other apps use Precompose Navigation).androidx.compose.ui:ui-tooling-preview
.Please confirm if it's OK to remove the Firebase in-app messaging feature, otherwise AppCompat and Glide must be kept in the app just for it.