Fix UI build due to Ktor upgrade#4572
Conversation
Ktor 3.4+ CIO engine uses Node.js sockets (node:net) for wasmJs, which is incompatible with browser webpack targets. Split the HTTP client engine per platform: ktor-client-js (fetch API) for wasmJs browser, ktor-client-cio for desktop/JVM only.
|
I don't know why this problem is (apparently?) only happening on that particular Docker nightly CI build. I don't know how to validate/test this change, to be honest. I'm looking for review! As an aside... I wish our build didn't require node/npm as it's another build/dependency system that requires configuration and that which can be problematic with some corporate environments with HTTP proxies, etc. Consequently I couldn't even really evaluate this PR on one of my machines, as I have a hack to my local config to not do the UI build because I can't there. |
Agree. I wonder if we could make the Kotlin AdminUI app a separate build in the monorepo, also with separate releases (standalone and js). Then our main build would somehow consume a pre-built artifact instead of building the UI app from scratch. |
janhoy
left a comment
janhoy
left a comment
There was a problem hiding this comment.
I have not tried to deeply understand the failure or the fix, but I asked Claude to do a code review on the PR and here it is:
The approach looks correct to me. Since getDefaultClient() in HttpClientUtils.kt calls HttpClient { ... } with no explicit engine, the engine is resolved by Ktor's auto-discovery from whatever is on each target's classpath. Moving ktor-client-cio to desktopMain keeps the CIO engine for the JVM/desktop target, and ktor-client-js provides the fetch-based engine for the wasmJs browser target — exactly what's needed.
The lockfile diff is a good confirmation the root cause is addressed: the ktor-network / ktor-network-tls wasm-js variants (which transitively brought in the node:net usage) are no longer on the wasmJs classpath. Tests use MockEngine via ktor-client-mock, so they're unaffected.
One minor ordering nit below. Otherwise LGTM.
| ktor-client-auth = { module = "io.ktor:ktor-client-auth" } | ||
| ktor-client-cio = { module = "io.ktor:ktor-client-cio" } | ||
| ktor-client-contentNegotiation = { module = "io.ktor:ktor-client-content-negotiation" } | ||
| ktor-client-js = { module = "io.ktor:ktor-client-js" } |
There was a problem hiding this comment.
Minor: the aliases here should be in alphabetical order
There was a problem hiding this comment.
Yes, I thought the linter would also complain about this?
jaykay12
left a comment
jaykay12
left a comment
There was a problem hiding this comment.
Not reviewing the PR codewise.
But these changes do fix the broken ./gradlew dev for sure. Verified.
| main branch | this branch |
|---|---|
 |
 |
…nce apache#4572 merges) Squashed changes from apache#4572 'Fix UI build due to Ktor upgrade' (dsmiley) to unblock CI/testing of this branch. Without it the solr:ui wasmJs browser webpack build fails on node:net, forcing -PdisableUiModule=true locally. THIS COMMIT IS TEMPORARY and must be dropped/reverted once apache#4572 is merged to main and this branch is rebased onto it.
…resources The Compose resource accessor generation tasks were not reliably wired as dependencies of the Kotlin compile tasks, notably compileKotlinWasmJs. On incremental or reordered builds the compile could fire before the accessors existed, failing with "Source file or directory not found" for the generated commonMainResourceAccessors (Drawable0/String0.commonMain.kt). Wire every compileKotlin* task to depend on all generateResourceAccessorsFor* tasks.
malliaridis
left a comment
malliaridis
left a comment
There was a problem hiding this comment.
Looks good to me. The change with the engine dependencies is correct and as far as I remember the better way.
I used the CIO engine for all targets as it was a single dependency that worked on all targets the best, with best feature support as well. By now things have changed over the time, and it may not be the best engine to use across all platforms.
About the asset issue, I was not aware, nor can I exactly follow the wiring here. But if it is confirmed to solve the issue, it's fine. Should not break anything at least.
I wish our build didn't require node/npm
Note that the AdminUI is using node under the hood, but still expects the build environment to be compatible with it. There is however another node integration in gradle/node.gradle which is probably replaceable.
I wonder if we could make the Kotlin AdminUI app a separate build in the monorepo, also with separate releases (standalone and js)
I will look into this. Normally it should be easy to do so by introducing a separate settings.gradle.kts file in that module and excluding it from the existing one. This would also allow us to split the module into UI feature modules in the long run, which I was considering too.
The tricky part with this is depending on the build artifacts of the admin UI without waiting for any build to happen. Probably via GitHub artifacts, but what about any other CI/CD pipelines? Will they be able to fetch the artifacts during builds?
I will investigate and create a draft PR for that. :)
4 participants
I observed the Docker nightly GHA based CI build is failing on main branch alone -- see here.
Claude's analysis:
This PR implements that change.