-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Bundle Compose Hot Reload Gradle plugin into the Compose Gradle plugin #5444
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: master
Are you sure you want to change the base?
Conversation
2195620
to
a800d3f
Compare
# A version of Gradle plugin, that will be published, | ||
# unless overridden by COMPOSE_GRADLE_PLUGIN_VERSION env var. | ||
deploy.version=9999.0.0-SNAPSHOT | ||
hotreload.version=1.0.0-beta08 |
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 guess Ii's supposed to be configured on CI, here is the place only for some placeholder
cc @igordmn
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.
It is better to configure it only here, not on CI.
Because it is a dependency version, not a version of the building component.
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.
It is usually better to define such versions in lib.versions.toml
, not in gradle.properties
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 have to have a place for version declaration that I can use in build script as well as in tests. I can't use lib.versions.toml
from tests. I see that versions are exported to sources via ComposeBuildConfig
generation. And ComposeBuildConfig
is generated based on gradle.properties
. Though I can use lib.versions.toml
from build.gradle.kts
and export it via ComposeBuildConfig
but, I am not sure here because it won't be aligned with other declarations.
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.
Though I can use lib.versions.toml from build.gradle.kts and export it via ComposeBuildConfig but, I am not sure here because it won't be aligned with other declarations.
It is not so important, but I suggested this because of the different nature from the build perspective - HotReload is hardcoded dependency that is built separately, Compose/Material3 on the other hand are non-hardcoded components passed from CI. That is why, it is probably better to have them configured differently.
From user (not build) perspective, they probably have the same nature - they are all versions of something provided by the plugin. That is why, it is probably better to have them in one place in ComposeBuildConfig
.
The edge between component/dependency and hardcoded/passed from CI is thin, so any solution in the end should work, choose one you think is better.
gradle-plugins/compose/src/test/test-projects/application/hotReload/build.gradle
Show resolved
Hide resolved
|
||
try { | ||
project.pluginManager.apply("org.jetbrains.compose.hot-reload") | ||
} catch (_: Exception) { |
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.
Catching any exception might swallow issues? How about catching only the exception we get when the plugin is missing?
override fun canRelocatePath(path: String?): Boolean { | ||
return super.canRelocatePath(path) && | ||
// do not relocate orchestration as its objects are used in serialization. | ||
path?.startsWith("org/jetbrains/compose/reload/orchestration/") == false |
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.
As of right now, we are locked out with this attempt:
We cannot shade the orchestration as this breaks java.io.Serializable, we cannot embedd it either because of classpath conflicts
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.
Waiting for the approach with the 'preferred' dependency
Compose Hot Reload Gradle plugin are bundled into the Compose Gradle plugin.
Fixes CMP-8885
Testing
Integration tests added
This should be tested by QA
Release Notes
Highlights - Multiple Platforms