Make some async api work on Scala.js and Native #12
Conversation
This PR breaks binary compatibility in Scala.js and Scala Native but doesn't touch binary compatibility for Scala JVM. Since the methods removed weren't linking on JS/Native this can be released as a patch release, until we find an API that works seamlessly in all the platforms. This deprecates Context.Impl.scheduler which cannot be implemented on Native and JS and suggests to use scheduleMsg instead.
|
@lolgab is it possible to duplicate less of the code? If the only thing that differs is |
|
It is possible if we change the API in a breaking way.
-def scheduleMsg[T](a: Actor[T],msg: T, delay: java.time.Duration)(implicit fileName: sourcecode.FileName, line: sourcecode.Line)
+def scheduleMsg[T](a: Actor[T],msg: T, delay: scala.concurrent.duration.FiniteDuration)(implicit fileName: sourcecode.FileName, line: sourcecode.Line)
Also I removed My idea was to live with this for a bit until we find an API that works on all platforms that we are happy with and release a breaking version. Let me know what approach you think is the best and I can try to implement it. :) |
|
Could we add an override that takes FiniteDuration? that way people who
want to use it cross-platform can do so, while those who have code on the
JVM using java.time can continue using the existing API without change
…On Wed, 15 Sep 2021 at 5:00 PM, Lorenzo Gabriele ***@***.***> wrote:
It is possible if we change the API in a breaking way.
We need to either:
- Change scheduleMsg signature from:
-def scheduleMsg[T](a: Actor[T],msg: T, delay: java.time.Duration)(implicit fileName: sourcecode.FileName, line: sourcecode.Line)+def scheduleMsg[T](a: Actor[T],msg: T, delay: scala.concurrent.duration.FiniteDuration)(implicit fileName: sourcecode.FileName, line: sourcecode.Line)
- or make the library depend on
https://github.com/cquiroz/scala-java-time on JS (Native not yet
supported) which is a big library and an overkill for this use case IMO.
Also I removed Context.Simple.threadPool, Context.makeThreadPool,
Context.ThreadPool, Context.TestThreadPool from JS and Native and we need
to find them another home if we want to duplicate less code.
My idea was to live with this for a bit until we find an API that works on
all platforms that we are happy with and release a breaking version.
Let me know what approach you think is the best and I can try to implement
it. :)
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#12 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHEB7ANUD4LX4FWE37CX73UCBOCPANCNFSM5D736X5A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Sadly you can't leave methods using |
|
Looks good to me I think |
This implements a different approach which is NOT binary compatible, but is source compatible and avoid duplicating code. The idea is to have mixins with different implementations for every object or trait that has functions existing on JVM only
|
@lihaoyi Ready for the final review. |
|
Tagging 0.2.0 sounds good. Castor is still a relatively young library, so we should expect it to evolve over time where necessary. Skimmed over the changes, looks good to me |
|
|
This PR breaks binary compatibility in Scala.js and Scala Native but
doesn't touch binary compatibility for Scala JVM.
Since the methods removed weren't linking on JS/Native this can be
released as a patch release, until we find an API that works seamlessly in
all the platforms.
This deprecates
Context.Impl.schedulerwhich cannot be implemented onNative and JS and suggests to use
scheduleMsginstead.scheduleMsgis removed from Scala.js and Scala Native. We should probablyadd an alternative using
scala.concurrent.durationso it can be implemented inall platforms, but since it wasn't working it is fine to remove for now since nobody
could use it either way.
./mill -i __.jvm.__.mimaReportBinaryIssuespasses all binary compatibility checks