Skip to content
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

Support wasmJs target #771

Merged
merged 10 commits into from
Jan 11, 2024

Conversation

IlyaGulya
Copy link
Contributor

@IlyaGulya IlyaGulya commented Jan 4, 2024

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
Copy link
Contributor

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

@IlyaGulya
Copy link
Contributor Author

Sure =)

@IlyaGulya IlyaGulya force-pushed the feature/add-wasmJs-target-support branch from 650e174 to 9f1534c Compare January 4, 2024 12:26
@IlyaGulya
Copy link
Contributor Author

IlyaGulya commented Jan 4, 2024

@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
Copy link
Contributor

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]

@IlyaGulya IlyaGulya force-pushed the feature/add-wasmJs-target-support branch from 479aff3 to f37282e Compare January 6, 2024 08:24
CherryPerry
CherryPerry previously approved these changes Jan 6, 2024
@CherryPerry
Copy link
Collaborator

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

@arkivanov
Copy link
Contributor

@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
Copy link
Collaborator

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.

@CherryPerry CherryPerry self-requested a review January 7, 2024 23:12
@arkivanov
Copy link
Contributor

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
Copy link
Contributor Author

I will take a look soon

@IlyaGulya
Copy link
Contributor Author

IlyaGulya commented Jan 10, 2024

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

@IlyaGulya IlyaGulya force-pushed the feature/add-wasmJs-target-support branch from e87de8c to d32fd31 Compare January 10, 2024 08:26
@IlyaGulya IlyaGulya force-pushed the feature/add-wasmJs-target-support branch from 5d97800 to e39b45f Compare January 10, 2024 08:56
@arkivanov
Copy link
Contributor

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

@IlyaGulya
Copy link
Contributor Author

@arkivanov I've fixed all issues

@IlyaGulya
Copy link
Contributor Author

IlyaGulya commented Jan 10, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor

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

@IlyaGulya
Copy link
Contributor Author

IlyaGulya commented Jan 10, 2024

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 ( )

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

@IlyaGulya
Copy link
Contributor Author

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

@IlyaGulya IlyaGulya force-pushed the feature/add-wasmJs-target-support branch from e39b45f to 1af5326 Compare January 10, 2024 11:11
@IlyaGulya
Copy link
Contributor Author

@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
Copy link
Contributor Author

Sorry, I will fix detekt complaints in a moment

@IlyaGulya IlyaGulya force-pushed the feature/add-wasmJs-target-support branch from 1af5326 to 11174af Compare January 10, 2024 11:54
@@ -0,0 +1,33 @@
package com.badoo.reaktive.scheduler

internal actual fun jsSetTimeout(task: () -> Unit, delayMillis: Int): TimeoutId =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can still return Int and avoid TimeoutId. This actual function could be as follows:

internal actual fun jsSetTimeout(task: () -> Unit, delayMillis: Int): Int =
    window.setTimeout(
        handler = { task().toJsReference() },
        timeout = delayMillis,
    )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about NodeJS runtime where setTimeout returns Timeout object instead of Int?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are correct. Let's use the current approach with expect/actual TimeoutId.

id =
globalThis.setTimeout(
{
timeoutIds.remove(id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still need to remove this id. The task is only executed once. Otherwise ids will accumulate and consume memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Year, sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the root of the bug 🙂
Someone wanted to remove id only for timeout, but somehow managed to do the same for interval =)

assertEquals(expectedResults, results)
// Required to pass test on NodeJS environment since runtime waits
// for all tasks to cancel or finish their work.
executor.cancel()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This callback is only called on success. It's better to dispose the executor as follows:

return checkTicks.toList()
    .doOnBeforeFinally(executor::dispose)
    .testAwait { ... }

@IlyaGulya IlyaGulya force-pushed the feature/add-wasmJs-target-support branch from 11174af to a4fe69f Compare January 10, 2024 13:32
@@ -0,0 +1,33 @@
package com.badoo.reaktive.scheduler

internal actual fun jsSetTimeout(task: () -> Unit, delayMillis: Int): TimeoutId =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are correct. Let's use the current approach with expect/actual TimeoutId.

@CherryPerry CherryPerry merged commit 61cc293 into badoo:master Jan 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support WASM target
3 participants