-
Notifications
You must be signed in to change notification settings - Fork 105
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
Don't use CrossVersion.for3Use2_13 in Scala.js #1054
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big kudos for the contribution! I had only one small comment.
The CI is down, so don't worry about it failing. After you make the change, I'll merge and release it.
@@ -31,7 +31,7 @@ object SbtShared { | |||
val sbt = latest212 | |||
val jvm = latest213 | |||
val cross = List(latest210, latest211, latest212, latest213, old3, js, sbt, jvm).distinct | |||
val crossJS = List(latest212, latest213, js).distinct | |||
val crossJS = List(latest212, latest213, stableLTS, js).distinct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be old3
. The reason is that Scastie can run all Scala versions and 3.0.0 is the one that we can use in all newer versions. If we use stableLTS, we won't be able to use versions below it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. I missed that (I thought you could only pick LTS and Next).
I do get an error compiling apiJS3
when I change that:
[error] error while loading $throws$package$,
[error] class file scala/runtime/$throws$package.class is broken, reading aborted with class dotty.tools.tasty.UnpickleException
[error] TASTy signature has wrong version.
[error] expected: {majorVersion: 28, minorVersion: 0}
[error] found : {majorVersion: 28, minorVersion: 1}
[error]
[error] This TASTy file was produced by a more recent, forwards incompatible release.
[error] To read this TASTy file, please upgrade your tooling.
[error] The TASTy file was produced by Scala 3.1.3-bin-nonbootstrapped
I'll see if I can find out what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I think I know the reason. So this is caused because the Scala version is lower than some library on classpath.
So the fix has to be something different. The exact cause why we need this cross compilation is for our runtime macros that we use in worksheet to display value decorations. The error you see comes from play json dependency.
If I may suggest something:
Leave the versions 2.12
, 2.13
and js
.
Then we should only handle the runtime-scala
dependency to always cross compile:
"org.scastie" %%% "runtime-scala" % "1.0.0-SNAPSHOT" cross CrossVersion.for3Use2_13
and handle the others in the way you've changed it to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that initially, but that causes another problem: runtime-scala
includes scalajs-dom
, so in this case it would pull the 2.13 version
So any Scala 3 library that includes scalajs-dom then fails. :(
Maybe one alternative would be to use "org.scastie" %%% "runtime-scala" % "1.0.0-SNAPSHOT" cross CrossVersion.for3Use2_13
if the scala target is lower than 3.3.0 and "org.scastie" %%% "runtime-scala" % "1.0.0-SNAPSHOT"
otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a fix for that in my Scala-CLI PR. I dropped play json and it should work. Until then I think there is nothing we can do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I wasn't aware of #997, that looks pretty exciting.
I'll eagerly wait for that, then 🙏
Fix #1024
This seems to be enough and, from my tests, both code compiled with Scala 3 and Scala 2.13 now works as expected.
I'm not sure if there's anything that I could have done to allow one to optionally use
CrossVersion.for3Use2_13
, but one can always just compile with 2.13 if that's the case.