fix: ScalaJS's KyoApp #807

HollandDM · 2024-11-01T17:35:52Z

resolve #784

HollandDM · 2024-11-01T17:36:21Z

It's quite late in my time zone, I'll put some descriptions tomorrow

hearnadam

Thanks for taking this on!

kyo-core/jvm/src/main/scala/kyo/KyoAppBasePlatformSpecific.scala

kyo-core/js/src/main/scala/kyo/KyoAppBasePlatformSpecific.scala

fwbrasil · 2024-11-01T19:23:35Z

kyo-core/js/src/main/scala/kyo/KyoAppBasePlatformSpecific.scala

+
+    final def main(args: Array[String]) =
+        this._args = args
+        if initCode ne null then initCode()


I think we could throw an exception in case the App has no run calls. I imagine some users might try to define def run like in other effect systems and it'd just not execute anything silently.

kyo-core/js/src/main/scala/kyo/KyoAppBasePlatformSpecific.scala

kyo-core/jvm/src/main/scala/kyo/KyoAppBasePlatformSpecific.scala

kyo-core/js/src/main/scala/kyo/KyoAppPlatformSpecific.scala

kyo-core/jvm/src/main/scala/kyo/KyoAppPlatformSpecific.scala

kyo-core/js/src/main/scala/kyo/KyoAppPlatformSpecific.scala

hearnadam

Do you mind adding a test with multiple runs if we don't have one already?

I think it can be as simple as building a list of runs

val x = ListBuffer...
object TestApp extends KyoApp:
  run { IO(x += 1) }
  run { IO(x += 2) }
TestApp.main(Array.empty)
assert(x == Seq(1, 2, 3....)

kyo-core/jvm/src/main/scala/kyo/KyoAppPlatformSpecific.scala

kyo-core/js/src/main/scala/kyo/KyoAppPlatformSpecific.scala

HollandDM · 2024-11-06T16:05:00Z

Updated the code a bit, and added a small test for KyoApp in js

HollandDM · 2024-11-07T08:04:39Z

Updated, fix and add one more test for KyoApp on JS

HollandDM · 2024-11-07T15:53:05Z

Updated, squash commits

hearnadam · 2024-11-06T16:08:50Z

kyo-core/shared/src/test/scala/kyo/KyoAppTest.scala

@@ -30,6 +31,16 @@ class KyoAppTest extends Test:
            runs <- ref.get
        yield assert(runs == 3)
    }
+    "multiple runs on JS" taggedAs jsOnly in run {


Both JVM and JS should pass this test. I notice we have a similar test, but it doesn't check ordering which is important. You can merge them if you want.

Suggested change

"multiple runs on JS" taggedAs jsOnly in run {

"multiple ordered runs" in run {

kyo-core/js/src/main/scala/kyo/KyoAppPlatformSpecific.scala

kyo-core/shared/src/main/scala/kyo/KyoApp.scala

hearnadam · 2024-11-07T16:08:41Z

kyo-core/shared/src/test/scala/kyo/KyoAppTest.scala

@@ -30,6 +34,17 @@ class KyoAppTest extends Test:
            runs <- ref.get
        yield assert(runs == 3)
    }
+    "multiple runs on JS" taggedAs jsOnly in {


This test is necessary for JVM and JS. We can delete the other one.

Suggested change

"multiple runs on JS" taggedAs jsOnly in {

"ordered runs" in {

hearnadam · 2024-11-07T16:10:06Z

kyo-core/shared/src/main/scala/kyo/KyoApp.scala


        final protected def args: Array[String] = _args

        private var _args: Array[String] = null

-        private val initCode = new ListBuffer[() => Unit]
+        protected var maybeProc: Maybe[() => Unit] = Absent


Can this be Iterable[() => Unit]? Or IterableOnce? I think the changes in this PR break multiple runs on JVM.

hearnadam · 2024-11-07T16:12:55Z

kyo-core/jvm/src/main/scala/kyo/KyoAppPlatformSpecific.scala

+            val result = IO.Unsafe.run(Abort.run(Async.runAndBlock(timeout)(handle(v)))).eval
+            printResult(result)
+        )
+        if maybeProc.isEmpty then


Won't this only be true once? I think we can make the maybeProc a generic Iterable:

private var initCode: Iterable[() => Unit] = Nil

Then we can just always update it. Both versions should be able to conform to this signature.

Scala JS:
initCode = maybeProc.toList
jvm
initCode = procListBuffer.toList

I think I understand what you're trying to say now, I'll try to update this.

kyo-core/js/src/main/scala/kyo/KyoAppPlatformSpecific.scala

HollandDM · 2024-11-07T17:09:04Z

kyo-core/jvm/src/main/scala/kyo/KyoAppPlatformSpecific.scala

+
+    final override protected def run[A: Flat](v: => A < (Async & Resource & Abort[Throwable]))(using Frame): Unit =
+        import AllowUnsafe.embrace.danger
+        initCode += (() =>


JVM side, code is pretty much the same

HollandDM · 2024-11-07T17:09:42Z

kyo-core/shared/src/test/scala/kyo/KyoAppTest.scala

@@ -30,20 +34,47 @@ class KyoAppTest extends Test:
            runs <- ref.get
        yield assert(runs == 3)
    }
-    "effects" taggedAs jvmOnly in {
+    "orderred runs" in {


update this test for both platforms

hearnadam · 2024-11-08T09:01:48Z

kyo-core/shared/src/test/scala/kyo/KyoAppTest.scala

@@ -30,20 +34,47 @@ class KyoAppTest extends Test:
            runs <- ref.get
        yield assert(runs == 3)
    }
-    "effects" taggedAs jvmOnly in {
+    "orderred runs" in {


Suggested change

"orderred runs" in {

"ordered runs" in {

hearnadam · 2024-11-09T22:28:31Z

kyo-core/js/src/main/scala/kyo/KyoAppPlatformSpecific.scala

+    final override protected def run[A: Flat](v: => A < (Async & Resource & Abort[Throwable]))(using Frame): Unit =
+        import AllowUnsafe.embrace.danger
+        val currentAsync: Unit < (Async & Abort[Throwable]) =
+            Async.timeout(timeout)(handle(v)).map(value => printResult(Result.success(value)))


If you put the timeout within the initiation of the Unit function, (when you update initCode) you will avoid some sleep fibers and potentially racy behavior.

I think it makes sense to timeout the whole application anyways.

I mimicked the code in JVM (timeout for each runs). Will updated this

hearnadam · 2024-11-09T22:49:39Z

kyo-core/js/src/main/scala/kyo/KyoAppPlatformSpecific.scala

+        maybePreviousAsync = maybePreviousAsync match
+            case Absent                 => Present(currentAsync)
+            case Present(previousAsync) => Present(previousAsync.map(_ => currentAsync))
+        if initCode.isEmpty then


I was initially wondering why this would work, when only the first fiber would be initialized, but then I realized the first fiber would begin the execution of the next...

In my testing, I think this code also causes repeatWithDelay to run forever. The run fiber is already complete when the timeout expires.

Regardless, I re-wrote this a bit which I think is a simplification:

package kyo import kyo.Maybe.Absent import kyo.Maybe.Present abstract class KyoAppPlatformSpecific extends KyoApp.Base[Async & Resource & Abort[Throwable]]: private var previous: Maybe[Unit < (Async & Abort[Throwable])] = Absent final override protected def run[A: Flat](v: => A < (Async & Resource & Abort[Throwable]))(using Frame): Unit = import AllowUnsafe.embrace.danger val current: Unit < (Async & Abort[Throwable]) = Abort.run(handle(v)).map(result => IO(printResult(result)).andThen(Abort.get(result)).unit) previous = previous match case Absent => Present(current) case Present(p) => Present(p.map(_ => current)) initCode = previous.map { async => () => val raced = Clock.repeatWithDelay(1.hour)(()).map { fiber => val race = Async.race(fiber.get, async) Async.timeout(timeout)(race) } val _ = IO.Unsafe.run(Async.run(raced)).eval }.toList end run end KyoAppPlatformSpecific

In KyoApp I made initCode a variable with a List for ease of conversion with Maybe.

protected var initCode: List[() => Unit] = List.empty

You can check if the Clock fiber is still running by changing () to a Console.println and increase the frequency of heartbeat.

hearnadam · 2024-11-09T22:51:04Z

kyo-core/shared/src/test/scala/kyo/KyoAppTest.scala

+            run { IO(x += 1) }
+            run { IO(x += 2) }
+            run { IO(x += 3) }


Let's mix in Async just for posterity:

run { Abort.delay(10.millis)(IO(x += 1)) } run { Async.delay(10.millis)(IO(x += 2)) } run { Async.delay(10.millis)(IO(x += 3)) }

fwbrasil · 2024-12-04T08:19:00Z

folks, anything pending to merge this? We can follow up with improvements if needed. It'd be nice to get the issue fixed.

HollandDM · 2024-12-04T14:21:30Z

I'll try to finalize this in the upcoming days, I think there aer only some little code conventions/styles left to adjust

HollandDM · 2024-12-05T06:55:20Z

@fwbrasil @hearnadam I've updated the code, it should be good now

fwbrasil · 2024-12-06T06:58:03Z

kyo-core/jvm/src/main/scala/kyo/KyoAppPlatformSpecific.scala

+
+import scala.collection.mutable.ListBuffer
+
+abstract class KyoAppPlatformSpecific extends KyoApp.Base[Async & Resource & Abort[Throwable]]:


The build is failing in Native. I think it's just a matter of moving this file to kyo-core/jvm-native/src/

updated and tested, should work now

fwbrasil · 2024-12-06T16:21:19Z

@HollandDM congrats on your first contribution 🙌

hearnadam reviewed Nov 1, 2024

View reviewed changes

fwbrasil reviewed Nov 1, 2024

View reviewed changes

kyo-core/js/src/main/scala/kyo/KyoAppBasePlatformSpecific.scala Outdated Show resolved Hide resolved

fwbrasil reviewed Nov 1, 2024

View reviewed changes

kyo-core/js/src/main/scala/kyo/KyoAppBasePlatformSpecific.scala Outdated Show resolved Hide resolved

fwbrasil reviewed Nov 1, 2024

View reviewed changes

kyo-core/js/src/main/scala/kyo/KyoAppBasePlatformSpecific.scala Outdated Show resolved Hide resolved

fwbrasil reviewed Nov 1, 2024

View reviewed changes

kyo-core/js/src/main/scala/kyo/KyoAppBasePlatformSpecific.scala Outdated Show resolved Hide resolved

fwbrasil reviewed Nov 1, 2024

View reviewed changes

kyo-core/jvm/src/main/scala/kyo/KyoAppBasePlatformSpecific.scala Outdated Show resolved Hide resolved

HollandDM commented Nov 5, 2024

View reviewed changes

kyo-core/js/src/main/scala/kyo/KyoAppPlatformSpecific.scala Outdated Show resolved Hide resolved

HollandDM commented Nov 5, 2024

View reviewed changes

kyo-core/jvm/src/main/scala/kyo/KyoAppPlatformSpecific.scala Outdated Show resolved Hide resolved

HollandDM requested review from fwbrasil and hearnadam November 5, 2024 15:08

HollandDM commented Nov 5, 2024

View reviewed changes

kyo-core/js/src/main/scala/kyo/KyoAppPlatformSpecific.scala Outdated Show resolved Hide resolved

hearnadam reviewed Nov 5, 2024

View reviewed changes

HollandDM force-pushed the ScalaJS-for-KyoApp branch from 66133fd to 53ae543 Compare November 6, 2024 16:01

HollandDM requested a review from hearnadam November 6, 2024 16:02

HollandDM force-pushed the ScalaJS-for-KyoApp branch 2 times, most recently from 87e52f1 to 60a85a5 Compare November 7, 2024 04:17

HollandDM force-pushed the ScalaJS-for-KyoApp branch from 60a85a5 to 535a895 Compare November 7, 2024 15:51

hearnadam reviewed Nov 7, 2024

View reviewed changes

HollandDM force-pushed the ScalaJS-for-KyoApp branch from 535a895 to bd2fb4a Compare November 7, 2024 17:08

HollandDM commented Nov 7, 2024

View reviewed changes

kyo-core/js/src/main/scala/kyo/KyoAppPlatformSpecific.scala Show resolved Hide resolved

HollandDM commented Nov 7, 2024

View reviewed changes

HollandDM force-pushed the ScalaJS-for-KyoApp branch from bd2fb4a to 855f8ec Compare November 7, 2024 17:15

HollandDM requested a review from hearnadam November 7, 2024 17:15

hearnadam reviewed Nov 9, 2024

View reviewed changes

HollandDM force-pushed the ScalaJS-for-KyoApp branch 2 times, most recently from 36ea6a1 to 169cded Compare December 5, 2024 06:51

HollandDM requested a review from hearnadam December 5, 2024 06:54

HollandDM force-pushed the ScalaJS-for-KyoApp branch from 169cded to 114f224 Compare December 5, 2024 07:06

hearnadam approved these changes Dec 5, 2024

View reviewed changes

fwbrasil reviewed Dec 6, 2024

View reviewed changes

[core]: fix: ScalaJS's KyoApp getkyo#784

2d9f6ca

HollandDM force-pushed the ScalaJS-for-KyoApp branch from 114f224 to 2d9f6ca Compare December 6, 2024 14:59

HollandDM requested a review from fwbrasil December 6, 2024 14:59

hearnadam approved these changes Dec 6, 2024

View reviewed changes

hearnadam merged commit 997bbcb into getkyo:main Dec 6, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ScalaJS's KyoApp #807

fix: ScalaJS's KyoApp #807

HollandDM commented Nov 1, 2024 •

edited

Loading

HollandDM commented Nov 1, 2024

hearnadam left a comment

fwbrasil Nov 1, 2024

hearnadam left a comment

HollandDM commented Nov 6, 2024

HollandDM commented Nov 7, 2024

HollandDM commented Nov 7, 2024

hearnadam Nov 6, 2024

hearnadam Nov 7, 2024

hearnadam Nov 7, 2024

hearnadam Nov 7, 2024

HollandDM Nov 7, 2024

HollandDM Nov 7, 2024

HollandDM Nov 7, 2024

hearnadam Nov 8, 2024

hearnadam Nov 9, 2024

HollandDM Nov 14, 2024 •

edited

Loading

hearnadam Nov 9, 2024

hearnadam Nov 9, 2024

fwbrasil commented Dec 4, 2024

HollandDM commented Dec 4, 2024

HollandDM commented Dec 5, 2024

fwbrasil Dec 6, 2024

HollandDM Dec 6, 2024

fwbrasil commented Dec 6, 2024

	"multiple runs on JS" taggedAs jsOnly in run {
	"multiple ordered runs" in run {

	"multiple runs on JS" taggedAs jsOnly in {
	"ordered runs" in {


		import scala.collection.mutable.ListBuffer

		abstract class KyoAppPlatformSpecific extends KyoApp.Base[Async & Resource & Abort[Throwable]]:

fix: ScalaJS's KyoApp #807

fix: ScalaJS's KyoApp #807

Conversation

HollandDM commented Nov 1, 2024 • edited Loading

HollandDM commented Nov 1, 2024

hearnadam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hearnadam left a comment

Choose a reason for hiding this comment

HollandDM commented Nov 6, 2024

HollandDM commented Nov 7, 2024

HollandDM commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HollandDM Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwbrasil commented Dec 4, 2024

HollandDM commented Dec 4, 2024

HollandDM commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwbrasil commented Dec 6, 2024

HollandDM commented Nov 1, 2024 •

edited

Loading

HollandDM Nov 14, 2024 •

edited

Loading