-
Notifications
You must be signed in to change notification settings - Fork 46
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
build: introduce a multi-platform project to host the new Compose UI #4115
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4115 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 2 2
Lines 90 90
Branches 3 3
======================================
Misses 90 90 ☔ View full report in Codecov by Sentry. |
9889fc0
to
aeead57
Compare
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.
let's make sure the build is clean
id("kotlin-multiplatform-convention") apply false | ||
kotlin("multiplatform") |
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.
id("kotlin-multiplatform-convention") apply false | |
kotlin("multiplatform") | |
id("kotlin-multiplatform-convention") |
If there are changes required to the convention, then we apply them to the convention
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.
While the convention is fine as is, it requires the js
target which is not available in this package. A possible solution is to being able to customize the convention, this needs then enough defaults so that the convention does not break everything and can be customized when needed. If that works for you I'll look into that.
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.
By changing the section from the kotlin-multiplatform-convention
plugin from this:
js {
browser()
nodejs()
}
to this:
// adding js only when directory `jsMain` exists
if (projectDir.resolve("src/jsMain").exists()) {
js {
browser()
nodejs()
}
}
It's possible to achieve this behavior, not sure it's most optimal one (but might be effective in the long run, just checking for the target to build before importing the target)
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.
Wait, why don't we have js? Isn't Compose supposed to run also in browser? This is also our main goal: an in-browser interface
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 runs on web, but not using the js
target, instead using the wasmJs
one. This is the only way to use the cross-platform interface: https://kotlinlang.org/docs/wasm-overview.html
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.
Oh, I see. I'd proceed as follows:
- create
withJS()
extension method in the convention, runningjs { browser() nodejs() }
- create
withWasmJS()
extension method in the convention, runningjs { browser() nodejs() }
- enable only JVM inside the convention
- update all other modules using the convention to call
withJS()
I would avoid fancy autodetection of folders. It can get very surprising, and the logics is reversed (if I have JS, then I may have the folder, not vice versa). I could (and I do) generate js for purely kotlin subprojects
34fa775
to
3d8fb39
Compare
2092f41
to
dbdaf5a
Compare
fun PatternFilterable.excludeGenerated() = exclude { "build${separator}generated" in it.file.absolutePath } | ||
withType<Detekt>().configureEach { excludeGenerated() } | ||
ktlint { filter { excludeGenerated() } } |
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 think this can be moved in the convention
*/ | ||
@OptIn(ExperimentalComposeUiApi::class) | ||
fun main() { | ||
ComposeViewport(document.body!!) { |
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.
ComposeViewport(document.body!!) { | |
ComposeViewport(checkNotNull(document.body) { "Reasonable error message" }) { |
@@ -122,6 +124,7 @@ spotbugs-annotations = "com.github.spotbugs:spotbugs-annotations:4.9.0" | |||
redux-kotlin-threadsafe = "org.reduxkotlin:redux-kotlin-threadsafe:0.6.1" | |||
svgsalamander = "guru.nidi.com.kitfox:svgSalamander:1.1.3" | |||
trove4j = "net.sf.trove4j:trove4j:3.0.3" | |||
androidx-lifecycle-runtime-compose = { group = "org.jetbrains.androidx.lifecycle", name = "lifecycle-runtime-compose", version.ref = "androidx-lifecycle" } |
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.
please use lexicographic ordering
@@ -167,3 +170,5 @@ scalafmt = "cz.alenkacz.gradle.scalafmt:1.16.2" | |||
shadowJar = "com.github.johnrengelman.shadow:8.1.1" | |||
taskTree = "com.dorongold.task-tree:4.0.0" | |||
jpackage = "org.panteleyev.jpackageplugin:1.6.0" | |||
compose-multiplatform = { id = "org.jetbrains.compose", version.ref = "compose-multiplatform" } | |||
compose-compiler = { id = "org.jetbrains.kotlin.plugin.compose", version.ref = "kotlin" } |
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.
lexicographic ordering
@@ -40,6 +40,7 @@ include( | |||
"alchemist-ui-tooling", | |||
"alchemist-swingui", | |||
"alchemist-web-renderer", | |||
"alchemist-composeui", |
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.
lexicographic order
Hi @Tbaile! 👋 |
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
Introducing new project that tries to give Alchemist a UI that is completely multi-platform across desktop apps and web.
This is an introductory pull requests that adds the project and the initial dependencies.