Conversation
There was a problem hiding this comment.
Pull request overview
This pull request upgrades the project from Kotlin 2.0.10 to Kotlin 2.3.0, implementing significant compatibility changes required for the new compiler version. The upgrade involves migrating to new Kotlin Gradle Plugin APIs, adopting standard browser APIs for web targets, and introducing a custom compiler plugin for handling external symbols across JS and WASM targets.
Changes:
- Upgrade Kotlin from 2.0.10 to 2.3.0 across all build files and dependencies
- Replace custom W3C type declarations with standard kotlinx-browser library (0.5.0)
- Introduce WebImport annotation and compiler plugin to unify JS/WASM external symbol handling
- Update Kotlin Gradle Plugin API usage (compilerOptions, definitionFile, outputModuleName, linkTaskProvider)
- Remove redundant
.toInt()calls and convert integer literals to unsigned where needed for stricter type checking - Remove obsolete compiler flags and suppressions that are no longer needed
Reviewed changes
Copilot reviewed 49 out of 55 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dependencies.toml | Updated Kotlin version to 2.3.0 and added kotlinx-browser dependency |
| skiko/buildSrc/gradle.properties | Updated Kotlin version for buildSrc |
| skiko/build.gradle.kts | Migrated to new KGP APIs (outputModuleName, compilerOptions), added webMain dependencies |
| skiko/buildSrc/.../NativeTasksConfiguration.kt | Updated to compilerOptions.configure and definitionFile APIs |
| skiko/buildSrc/.../WasmTasksConfiguration.kt | Removed ExperimentalWasmDsl opt-in, reorganized imports |
| skiko/src/webMain/.../W3CSubset.kt | Deleted custom W3C declarations in favor of kotlinx-browser |
| skiko/src/webMain/.../Actuals.web.kt | Added WebImport annotation, switched to kotlinx.browser |
| skiko/src/webMain/.../*.kt | Replaced custom w3c imports with kotlinx.browser and org.w3c.dom |
| skiko/src/webMain/.../Resources.web.kt | New unified implementation for web resource loading |
| skiko/src/jsMain/.../Resources.js.kt | Simplified to delegate Int8Array conversion |
| skiko/src/wasmJsMain/.../Resources.wasmJs.kt | Deleted, consolidated to webMain |
| skiko/src/jsMain/.../Actuals.js.kt | Changed ExternalSymbolName to use WebImport |
| skiko/src/wasmJsMain/.../Actuals.wasm.kt | Changed ExternalSymbolName to use WebImport |
| skiko/src/commonTest/.../TestHelpers.kt | Removed OPTIONAL_DECLARATION_USAGE_IN_NON_COMMON_SOURCE suppressions |
| skiko/src/commonTest/.../testSamples.kt | Removed redundant .toInt() calls on integer literals |
| skiko/src/commonTest/.../CanvasTest.kt | Removed redundant .toInt() calls on 0x00000000 literals |
| skiko/src/webTest/.../TestUtils.web.kt | Added TestReturnType typealias for shared web test infrastructure |
| skiko/src/nativeMain/.../Resources.native.kt | Changed fread parameter from 1 to 1u for unsigned literal |
| skiko/src/iosTest/.../Utils.kt | Changed fread parameter from 1 to 1u |
| skiko/src/darwinMain/.../Dispatchers.kt | Changed dispatch_get_global_queue parameter from 0 to 0u |
| skiko/import-generator/.../ImportGeneratorTransformer.kt | Added support for WebImport annotation and JsName generation |
| skiko/import-generator/.../ImportGeneratorRegistrar.kt | Added pluginId property |
| skiko/import-generator/.../ImportGeneratorExtension.kt | Updated to export awaitSkiko directly instead of nested in api object |
| samples/SkiaWebSample/build.gradle.kts | Updated to new source set DSL and added browser dependency |
| samples/SkiaWebSample/gradle.properties | Updated Kotlin version to 2.3.0 |
| samples/SkiaWebSample/settings.gradle.kts | Added kotlinx-browser dependency declaration |
| samples/SkiaWebSample/.../App.web.kt | Simplified to use document.getElementById directly |
Comments suppressed due to low confidence (2)
skiko/src/webMain/kotlin/org/jetbrains/skiko/Actuals.web.kt:19
- There is an extra space before the colon in the parameter definition. It should be
name: Stringinstead ofname : Stringto follow Kotlin coding conventions.
skiko/src/webMain/kotlin/org/jetbrains/skiko/Actuals.web.kt:4 - The import
org.w3c.dom.HTMLElementis unused in this file and should be removed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| jetbrainsRuntime-api = { module = "org.jetbrains.runtime:jbr-api", version.ref = "jetbrainsRuntime-api" } | ||
|
|
||
| kotlinx-browser = { module = "org.jetbrains.kotlinx:kotlinx-browser", version.ref = "kotlinxBrowser" } |
There was a problem hiding this comment.
There is a trailing space after the closing brace on this line. Remove the trailing whitespace.
| kotlinx-browser = { module = "org.jetbrains.kotlinx:kotlinx-browser", version.ref = "kotlinxBrowser" } | |
| kotlinx-browser = { module = "org.jetbrains.kotlinx:kotlinx-browser", version.ref = "kotlinxBrowser" } |
| get() = wasmImport("setup.mjs") | ||
|
|
||
| private val Project.setupReexportMjs | ||
| get() = wasmImport("js-reexport-symbols.mjs") |
There was a problem hiding this comment.
Looks like this is not needed anymore?
Judging based on the below changes where this is removed:
if (reexportPath == null) {
writer.appendLine("window['${symbolName}'] = (...a) => loadedWasm._[\"${symbolName}\"](...a)")
}
| } | ||
|
|
||
| @OptIn(ExperimentalWasmJsInterop::class) | ||
| actual suspend fun loadBytesFromPath(path: String): ByteArray { |
There was a problem hiding this comment.
I just realized that this function is used only in the tests but we expose it in the public API by mistake.
| tasks.withType<KotlinNativeCompile>().configureEach { | ||
| // https://youtrack.jetbrains.com/issue/KT-56583 | ||
| compilerOptions.freeCompilerArgs.add("-XXLanguage:+ImplicitSignedToUnsignedIntegerConversion") | ||
| //compilerOptions.freeCompilerArgs.add("-XXLanguage:+ImplicitSignedToUnsignedIntegerConversion") |
There was a problem hiding this comment.
is this line commented intentionally?
https://youtrack.jetbrains.com/issue/KT-56583 is fixed, can the line be removed completely?
There was a problem hiding this comment.
this line was commented intentionally - I just wanted to discuss with our fellow composers whether it's needed but my understanding that it is indeed no longer needed so I'll just remove it
No description provided.