search

badoo / Reaktive

Kotlin multi-platform implementation of Reactive Extensions
Apache License 2.0
1.18k stars 58 forks source link

Support wasmJs target #771

Closed IlyaGulya closed 10 months ago

IlyaGulya commented 10 months ago

Fixes #767

In this PR:

  1. I've shared most of js sources with wasmJs by introducing the jsWasmJsCommon sourceSets.
  2. Enabled gradle configuration cache
  3. Upgraded Kotlin Coroutines to 1.8.0-RC2
arkivanov commented 10 months ago

Thanks! I've been working on this, and been waiting for stable coroutines version. Could you please sync with my branch? https://github.com/badoo/Reaktive/compare/master...arkivanov:Reaktive:wasm

IlyaGulya commented 10 months ago

Sure =)

IlyaGulya commented 10 months ago

@arkivanov I've cherry-picked changes from your branch which were relevant:

  1. Migration to kotlin-stdlib and kotlin-test dependencies from sourceSet-specific ones
  2. RepeatWhenTest changes
  3. Introduction of AsyncTestResult and relevant changes to testAwait extension as well as TestAwaitTest.
arkivanov commented 10 months ago

Detekt seems failing.

> Task :sample-mpp-module:detekt FAILED
/home/runner/work/Reaktive/Reaktive/sample-mpp-module/src/commonMain/kotlin/com/badoo/reaktive/samplemppmodule/Counter.kt:40:1: Needless blank line(s) [NoConsecutiveBlankLines]
/home/runner/work/Reaktive/Reaktive/sample-mpp-module/src/commonMain/kotlin/com/badoo/reaktive/samplemppmodule/Feature.kt:7:1: Unused import [NoUnusedImports]
/home/runner/work/Reaktive/Reaktive/sample-mpp-module/src/commonMain/kotlin/com/badoo/reaktive/samplemppmodule/Counter.kt:42:78: This expression contains a magic number. Consider defining it to a well named constant. [MagicNumber]
CherryPerry commented 10 months ago

@arkivanov WDYT about merging/releasing? Merge now or later when coroutines are not RC? The same for the release process.

arkivanov commented 10 months ago

@arkivanov WDYT about merging/releasing? Merge now or later when coroutines are not RC? The same for the release process.

I think we could merge now and release 2.1.0-beta01.

CherryPerry commented 10 months ago

wasmJsBrowserTest task hangs on my system (macOS). Looks like the same is happening on CI.

jsBrowserTest at least says Please make sure that you have installed browsers..

Should we also enable nodejs() for WASM too? wasmJsNodeTest does not hang but throws CompileError: WebAssembly.Module(): invalid value type 0x71 @+389.

You can debug CI by creating sub-bracnch from yours and opening PR to master of your fork.

arkivanov commented 10 months ago

wasmJsBrowserTest task hangs on my system (macOS). Looks like the same is happening on CI.

jsBrowserTest at least says Please make sure that you have installed browsers..

Should we also enable nodejs() for WASM too? wasmJsNodeTest does not hang but throws CompileError: WebAssembly.Module(): invalid value type 0x71 @+389.

You can debug CI by creating sub-bracnch from yours and opening PR to master of your fork.

I think for now we don't need nodejs, Wasm with nodejs is trickier. There must be an issue with tests, as I have Wasm browser running on CI successfully in other projects.

IlyaGulya commented 10 months ago

I will take a look soon

IlyaGulya commented 10 months ago

Follow up: I'm still fixing this. Linux build hangs on jsNodeTest task and I haven't figured out why yet. 🙂 UPD: Seems that it's an issue with MainScheduler periodic task test

arkivanov commented 10 months ago

I'm able to reproduce it locally, I can take a look.

IlyaGulya commented 10 months ago

@arkivanov I've fixed all issues

IlyaGulya commented 10 months ago

@arkivanov Please take a look at my changes inside of MainScheduler.

There was a bug (as far as I can tell, maybe I'm wrong 🙂 ) - interval and timeout ids were removed from list of interval ids/timeout ids immediately after task is executed first time. That's a strange logic, I've removed it.

arkivanov commented 10 months ago

@arkivanov Please take a look at my changes inside of MainScheduler.

There was a bug (as far as I can tell, maybe I'm wrong 🙂 ) - interval and timeout ids were removed from list of interval ids/timeout ids immediately after task is executed first time. That's a strange logic, I've removed it.

It appears that a Node test hangs if there are any pending task (that are not executed yet). I was able to fix the test by just removing this line. Looks like this is an actual bug, as we are removing the interval (periodic) task id after the first execution, and so later it can't be cancelled. What do you think?

arkivanov commented 10 months ago

I think we can revert the rest of the unrelated changes after the commit f37282e?

IlyaGulya commented 10 months ago

I think we can revert the rest of the unrelated changes after the commit f37282e?

Are you sure it's unrelated?

  1. MainScheduler implementation for JS and WASM has different timeout id types. WASM does not support dynamic type, so I have to use JsAny instead. Int is not correct and works only because of dynamic nature of JS. On NodeJS setTimeout and setInterval returns Timeout object instead of Int.
  2. For me test still hangs if I don't cancel the executor after equality check (https://github.com/badoo/Reaktive/blob/e39b45f674b3c7d9f7f522662028b89101f202fc/reaktive/src/jsCommonTest/kotlin/com/badoo/reaktive/scheduler/MainSchedulerTest.kt#L74)

I can squash some commits to make history cleaner, if you want

IlyaGulya commented 10 months ago

Hmm, I will refactor the MainThreadExecutor once again in a moment

IlyaGulya commented 10 months ago

@arkivanov I once again refactored a bit. Now there's only platform-specific TimeoutId class and js functions implementations. MainThreadExecutor is now common as before.

IlyaGulya commented 10 months ago

Sorry, I will fix detekt complaints in a moment