cloudflare / kotlin-worker-hello-world

Apache License 2.0
56 stars 11 forks source link

Kotlin Coroutines support? #2

Closed linux-china closed 4 years ago

linux-china commented 4 years ago

Upgrade worker to Kotlin 1.4.0 and all is good. But failed to run worker with kotlinx:kotlinx-coroutines-core-js. Cloudflare runtime compatible problem or my code problem?

build.gradle.kts:

    implementation("org.jetbrains.kotlinx:kotlinx-coroutines-core-js:1.3.9")

Example code:

import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.promise
import org.w3c.fetch.Response
import org.w3c.fetch.ResponseInit
import org.w3c.workers.FetchEvent

fun main() {
    addEventListener("fetch") { event: FetchEvent ->
        event.respondWith(handleEvent(event))
    }
}

fun handleEvent(event: FetchEvent) = GlobalScope.promise {
    val headers: dynamic = object {}
    headers["content-type"] = "text/plain"
    Response(
        "Hello Kotlin Worker",
        ResponseInit(headers = headers)
    )
}
koeninger commented 4 years ago
bash-3.2$ wrangler tail
�  Setting up log streaming from Worker script "projectname". Using ports 8080 and 8081.
Now prepared to stream logs.
{"outcome":"exception","scriptName":null,"exceptions":[{"name":"Error","message":"addEventListener(): useCapture must be false."

https://github.com/Kotlin/kotlinx.coroutines/blob/1b34e1c7dd6207d7683c307bae0b934a3dc18d09/kotlinx-coroutines-core/js/src/JSDispatcher.kt#L103

Editing the generated index.js file to change this line

this.window_0.addEventListener("message",(e=this,function(t){return t.source==e.window_0&&t.data==e.messageName_0&&(t.stopPropagation(),e.process()),l}),!0)

to pass false instead of !0 lets the worker run successfully, so that does seem to be the issue.

That being said, what is your use case for kotlin coroutines on Workers? It looks like a 5x size penalty in the generated javascript.

linux-china commented 4 years ago

@koeninger it works now. you did help me :) You know lots of Kotlin frameworks use Kotlinx.coroutines, such as Ktor etc, and suspend method is more friendly than JS Promise. It's true that the final bundle size is some big with Kotlin & coroutines runtime, but still be acceptable. I adjust compiler to use js(IR) and final bundle size just half now.

js(IR) {
        browser {
            binaries.executable()
            testTask {
                useMocha()
            }
        }
    }
LouisCAD commented 4 years ago

Editing the generated index.js file to change this line

this.window_0.addEventListener("message",(e=this,function(t){return t.source==e.window_0&&t.data==e.messageName_0&&(t.stopPropagation(),e.process()),l}),!0)

to pass false instead of !0 lets the worker run successfully, so that does seem to be the issue.

Does it mean one would need to edit the generated code manually after each compilation to have it run on cloudflare worker?

linux-china commented 4 years ago

@LouisCAD you can add some code in build.gradle.kts for replacement. My code:

tasks.register("buildWorker") {
    dependsOn("browserProductionWebpack")
    doLast {
        file("$projectDir/dist/").mkdirs()
        /* Kotlin js output assumes window exists, which it won't on Workers. Hack around it */
        val projectJsCode = file("$projectDir/build/distributions/${rootProject.name}.js")
            .readText()
            // bug fix https://github.com/cloudflare/kotlin-worker-hello-world/issues/2
            .replace("e.process()),l}),!0", "e.process()),l}),false") //legacy
            .replace("}),!0)}", "}),false)}") // IR
        file("$projectDir/dist/index.js").writeText("const window=this; $projectJsCode")
    }
}
LouisCAD commented 4 years ago

@linux-china Shouldn't you just use js { nodejs() } instead of js() or js { browser() } plus this hackery? Or doesn't it work?

linux-china commented 4 years ago

you should do the replacement too with following configuration.

kotlin {
    js(IR) {
        nodejs {
            testTask {
                useMocha()
            }
        }
        binaries.executable()
    }
}
LouisCAD commented 4 years ago

The replacement is still needed when building for the nodejs() target? If so, it's a bug that need to be reported?