Update project build configuration and dependencies#514
Update project build configuration and dependencies#514programadorthi merged 31 commits intoadrielcafe:mainfrom
Conversation
…guration logic to convention plugins
gradle/libs.versions.toml
Outdated
| mavenPublish = "0.30.0" | ||
| composeMultiplatform = "1.7.1" | ||
| binaryCompatibilityValidator = "0.16.3" | ||
| atomicfu = "0.26.1" |
There was a problem hiding this comment.
Fyi, this will currently cause an issue because compose expects (and forces) 0.23.2.
Seems to be a known issue https://youtrack.jetbrains.com/issue/CMP-5831
There was a problem hiding this comment.
I think it's compose multiplatform problem rather than voyager. Anyway I tested voyager with cmp 1.8.0-rc01 and it seems all works fine.
I intended to update this PR when cmp 1.8.0 will be released.
|
@Vodes why did you change from 8.2 => 8.11.1 It is to new mine one still using 8.7.2 |
Because its good practice to always update dependencies, Gradle is always meant to be up-to-date for the improvements in speed and bugfixes. |
|
@Syer10 has this fork or pr merge to master repo veyoger? |
# Conflicts: # samples/multiplatform/build.gradle.kts
|
Just updated this PR, what did I done more:
There is still some work to be done, e.g. update ktlint and resolve problems that it finds and update samples. But this is out of scope of this PR, so I intended to do this after this PR is merged. @devnatan, @DevSrSouza can you please take a look at this PR. |
programadorthi
left a comment
There was a problem hiding this comment.
Can you share evidences of all samples working after the whole changes?
|
|
||
| # IntelliJ | ||
| *.iml | ||
| *.focus No newline at end of file |
There was a problem hiding this comment.
.focus files are generated by dropbox plugin.
There was a problem hiding this comment.
I think we can remove the dropbox plugin, voyager project is a small project for using the plugin honestly.
|
|
||
| if (isAndroidLibraryModule()) { | ||
| val proguardFilename = "consumer-rules.pro" | ||
| if (layout.projectDirectory.file(proguardFilename).asFile.exists()) { |
There was a problem hiding this comment.
You don't need check if exists. AGP ignores the file whether it doesn't exist.
There was a problem hiding this comment.
AGP will print that certain proguard files not found:
Supplied consumer proguard configuration does not exist: /Users/dzmitry/AndroidStudioProjects/voyager/voyager-bottom-sheet-navigator/consumer-rules.pro
If you ok with this I remove an if clause.
There was a problem hiding this comment.
You removed the empty files. That is the reason to complain about missing it.
Is there any advantage to remove the files instead of putting the check for them?
There was a problem hiding this comment.
In my opinion a project has less clutter when it doesn't have files that don't affect anything.
| } | ||
| } | ||
|
|
||
| androidTarget { |
There was a problem hiding this comment.
Does it makes sense having androidTarget in a file that have desktop configuration?
| plugins.apply("org.jlleitschuh.gradle.ktlint") | ||
|
|
||
| configure<KtlintExtension> { | ||
| version.set("0.47.1") |
There was a problem hiding this comment.
Can you get the value from version catalog?
| add(projects.samples.multiModule.navigation) | ||
| } | ||
| with(nonPublicMarkers) { | ||
| add("cafe.adriel.voyager.core.annotation.InternalVoyagerApi") |
There was a problem hiding this comment.
Why removed ExperimentalVoyagerApi?
There was a problem hiding this comment.
Actually code marked with ExperimentalVoyagerApi is a part of public API surface of the voyager. So it shouldn't be treated as non-public and developer need to be aware about changes in this code.
There was a problem hiding this comment.
The string is with spaces, does this is correct? Second is that ExperimentalVoyagerApi is meant to be not in the API Surface because it should be a API that can be change at any time, the project Binary Compatibility should try to always be incremental and not change existing APIs, this is why the annotation is not there in the first place.
There was a problem hiding this comment.
I understand your reasoning but API marked with this annotation is still public. I think developer should be aware when doing changes even to this code as consumers still can depend on it.
There was a problem hiding this comment.
Samples is not a good name to this catalog because it'll conflicts with the samples folder and creating confusion from we're getting the dependencies. Keeping all versioning in an unique file isn't a problem.
There was a problem hiding this comment.
Can you please explain what conflicts do you mean? Samples modules are accessed through projects.samples.* while dependencies just with samples.*.
Maybe libsSamples.* or samplesLibs.* or demo.* will be a better naming?
I think it's good to have more fine-grained control over what dependencies you add to the library. Now it's not possible to 'accidentally' add new dependency from common catalog. With separate catalogs you need to make a mindful decision to add it to to libs catalog and it's becomes more clear to the reviewer when new dependency were added to library.
There was a problem hiding this comment.
Maybe samplesCatalog? I'm not good with names. For the first read I made assumptions between projects.samples and samples.*. Then a I realized that was a catalog.
| android:name=".App" | ||
| android:label="@string/app_name" | ||
| android:theme="@style/Theme.AppCompat.Light.NoActionBar" | ||
| android:theme="@android:style/Theme.Material.Light.NoActionBar" |
There was a problem hiding this comment.
Well you can change directly to androidx.material theme.
There was a problem hiding this comment.
It's a compose app which uses compose theme instead of xml one. So I removed appcompat dependency and used built-in theme.
settings.gradle.kts
Outdated
| gradlePluginPortal() | ||
| mavenLocal() |
There was a problem hiding this comment.
These two guys will increase the resolution. Why they are here?
There was a problem hiding this comment.
Agreed, I'll remove them
| jvmTest.dependencies { | ||
| implementation(libs.junit.api) | ||
| runtimeOnly(libs.junit.engine) | ||
| implementation(compose.runtime) |
There was a problem hiding this comment.
Voyager uses compileOnly to avoid transitive dependency at compile time.
There was a problem hiding this comment.
Seems using compileOnly for compose dependencies break compilation for certain targets, here the warning I get:
A compileOnly dependency is used in targets: Kotlin/JS, Kotlin/Native, Kotlin/Wasm.
Dependencies:
- org.jetbrains.compose.runtime:runtime-saveable:1.8.1 (source sets: iosArm64Main, iosSimulatorArm64Main, iosX64Main, jsMain, macosArm64Main, macosX64Main, wasmJsMain)
- org.jetbrains.compose.runtime:runtime:1.8.1 (source sets: iosArm64Main, iosSimulatorArm64Main, iosX64Main, jsMain, macosArm64Main, macosX64Main, wasmJsMain)
Using compileOnly dependencies in these targets is not currently supported, because compileOnly dependencies must be present during the compilation of projects that depend on this project.
To ensure consistent compilation behaviour, compileOnly dependencies should be exposed as api dependencies.
Furthermore, it is not possible to use voyager without using compose, why we should to avoid making a transitive dependency here? We not strictly enforce compose version so consumer just will use more recent version of compose.
I also checked out decompose for reference and it using implementation without any problems as it seems.
There was a problem hiding this comment.
The reason why this is showing is because of the change in the gradle.properties. There is a flag that the project had enable.
Can you hole back this? for now? This is a relative big change that I need to check with some Kotlin folks to see if the relocation of Compose KMP and Android libraries of Compose does not have the past issues anymore.
| private fun getHooks(): List<ProvidedValue<*>> { | ||
| atomicAppContext.compareAndSet(null, LocalContext.current.applicationContext) | ||
| atomicParentLifecycleOwner.compareAndSet(null, LocalLifecycleOwner.current) | ||
| atomicParentLifecycleOwner.compareAndSet(null, LocalLifecycleOwner.current) // deprecated |
There was a problem hiding this comment.
Remove the comment
|
When can this will merge to master? |
|
I think soon it'll be merged. It's a good contribution. Just some adjustments and done. |
|
@dzmpr i found bugs regarding to parseable serialize. error : when time you off the screen and come back to screen error about parse. so i come out this solution but don't know how to constribute: and i hope that |
|
@hafizkloudius014 Screen is JavaSerializable by default. More details here. |
|
Is this pr good to go to merge? |
| eachPlugin { | ||
| if (requested.version == null) return@eachPlugin | ||
| when (requested.id.id) { | ||
| "com.android.tools.build" -> { | ||
| useModule("${requested.id.id}:gradle:${requested.version}") | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Any reason why this is needed?
There was a problem hiding this comment.
By default Gradle to resolve plugin artifact by plugin id uses plugin id as group id and add .gradle.plugin suffix to plugin id to construct artifact id. So plugin "pluginId" version "1.0.0" becomes "pluginId:pluginId.gradle.plugin:1.0.0" artifact.
But AGP build plugin has custom artifact id which we need to override.
voyager-core/src/commonWebMain/kotlin/cafe.adriel.voyager.core/screen/ScreenKey.web.kt
Show resolved
Hide resolved
|
edit: |
…e collections compilation, fix Lifecycle KMP
|
I think now we ready to merge and release new version |
|
@DevSrSouza Hey, did you get a chance to test the PR locally with |
|
Since July of 2025 OSS sonatype repository is out of service so all new publications should go through cetral portal. I updated publishing configuration, now we need to migrate voyager namespace to center portal (here short instruction and here is official one) and update project secrets ( |
|
hi it is already july, it is ok can merge this pr? thanks you. |
|
nice it approved. may i know what is latest version of this library? thank you |
|
Any updates? |
|
hi @programadorthi and @DevSrSouza when can get the approval? |
|
Any updates on this PR? |
|
@DevSrSouza Hi, can you please take a look at this PR? There is one step left to release and unblock other improvements for the lib. |
|
hi @programadorthi should be increase the number of version library before can merge to main? |
|
Hi @programadorthi, it seems that publish of new version is failed. Will it be fixed? |
|
hi @programadorthi error: |
|
I think this #514 (comment) may be a problem. |
|
ok then their problem not migrate the portal. @dzmpr |
Hi there!
I have some troubles with m2 bottom sheet navigator so I decided to add m3 bottom sheet navigator and solve this problem for me and for others. But when I started working on it I saw that voyager uses compose multiplatform 1.6.11 when I needed 1.7. One after another I'm ended up made some improvement to project build configuration and updated all dependencies (not only compose kmp).
So, what's have been done:
.gitignoreandcustomer.proguard.profiles.buildSrcand replace it withbuild-configincluded build. WithbuildSrcthere are problems with using version catalogs and no pros.appcompat,compose-runtimeSaveable,compose-compiler,compose-animations, and maybe something else). Note: this compose dependencies are not multiplatform and only used in samples.com.benasher44:uuiddependency with kotlin stdlib implementation which is available from kotlin 2.0.compileOnlyfor compose dependencies with plainimplementationas now seems all working fine but withcompileOnlycompiler threw warnings about js targets.What's I left behind:
jvm("desktop")target which triggers compiler warning).ExperimentalVoyagerApifromnonPublicMarkers(I consider this is a mistake, because it' actually public api, just experimental). So if you are okay with this I update api with additional commit.I've tested all samples on android and following targets of multiplatform sample: js, wasm, macos (not sure what target).
So I'm waiting for review, and open for any questions if you have one.