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

Scala 3 support #494

Merged
merged 30 commits into from
Nov 1, 2021
Merged

Scala 3 support #494

merged 30 commits into from
Nov 1, 2021

Conversation

oleg-py
Copy link
Contributor

@oleg-py oleg-py commented Jul 28, 2021

@Asamsig has largely done cross-build setup, I've managed to, for the most part, port the reader/writer and other macros used to Scala 3. VR and native modules still don't compile since they use @react. I didn't test those extensively, just made sure it works with existing test suite (wherever @react isn't used)

The big sneaky change is that js.| is now Scala 3 union type, which behaves very differently (more natural wrt usage and more crazy when it comes to macros and implicit resolution).

There are also a lot of warnings related to machinery used for attribute type-safety, since type projections of form T#tagType are disallowed. The code currently compiles with -source:3.0-migration flag though, but I'm unsure if that's going to actually work in user-land (and that flag is really bad for some Scala 3 features and prevents some new syntax from working, so slinky users wouldn't want that).

Not too sure what the plans are for @react-style DSL. It would definitely be a huge pain for me to write code like Input.component(Input.Props(...)). I've been looking into making that convenient, and while you can't synthesize members, it's potentially possible to use Dynamic with a macro. I, however, was unable to figure out a macro rewrite from Input(...) to that above construct that would support named parameters.

Asamsig and others added 18 commits July 21, 2021 19:40
Dependencies have been updated to versions supporting Scala 3, IntelliJ manages to sync the project, this is a nice first step.

Introduces a new source folders `scala-2` & `scala-3`, for now the assumption is that `scala-2.13+` will be included for Scala 3.

Set Scala 3 as default version.
Read/Write is now split between Scala 2&3, Scala 2 compiles as before, Scala 3 is still missing an implementation.
Macros in Core is now split between Scala 2&3, Scala 2 compiles as before, Scala 3 is still missing an implementation.
@Asamsig
Copy link
Contributor

Asamsig commented Jul 28, 2021

Very impressive, great to see that you were able to get support for recursive case classes, value classes and default values, is there any outstanding issues then for read/write? I guess opaque types and Scala 3 enums might not work yet, but most important thing now is parity with Scala 2, in my opinion at least.

-source:3.0-migration should only be temporary, we should try to migrate all incompatible code to Scala 3, would be great with a MVP though, since I imagine it might be quite a chore.

@shadaj
Copy link
Owner

shadaj commented Jul 28, 2021

This is absolutely incredible! Am wrapping up my summer internship this/next week, but will try to look through the details this weekend!

@oleg-py
Copy link
Contributor Author

oleg-py commented Jul 29, 2021

@Asamsig

is there any outstanding issues then for read/write?
Not really on a surface level, but I specifically haven't done any checking for complex cases like classes with multiple constructors, type parameters, etc.

I guess opaque types and Scala 3 enums might not work yet
Never checked those, but enums should be able to work with mirrors (well, maybe not GADTs). Opaque types might be workable with the same approach I used for value types (custom mirror-like type providing conversion/type info with a macro implicit), but they would all "work" anyway with a fallback reader/writer.

@shadaj thanks, and good luck!

build.sbt Outdated Show resolved Hide resolved
@shadaj
Copy link
Owner

shadaj commented Aug 8, 2021

I think for the first pass, we should ignore @react and just get the basics working. for T#tagType, I wonder if we could just use regular dependent type syntax and refer to attr.tagType? I remember that we had to use T#tagType to get around some issues with dependent types in Scala 2.

@oleg-py
Copy link
Contributor Author

oleg-py commented Aug 14, 2021

Looks like Scala.js is only available on 2.12.14 since 1.5.1, and I'm not sure what's the lowest version that targets Scala 3. There's no 0.6.x for the most modern versions of Scala 2.12, 2.13 and 3.x either. I can downgrade Scala-2 versions back to where 0.6 existed and skip building for 3 if ScalaJS is not 1.x, though not sure if maintaining the 0.6 build is worth the trouble.

No idea what to do with dependent types yet - the thing is, we need to have a value (maybe an erased one would work) to project the type out. So the type supports[T <: Tag] = AttrPair[attrType] => AttrPair[T#tagType] won't work. Maybe something in the new features like polymorhpic values and/or context functions that could replace it though

@shadaj
Copy link
Owner

shadaj commented Aug 14, 2021

@oleg-py I'd be fine with dropping 0.6 builds, since this PR will be part of the 0.7.0 release and by now hopefully most users will have moved over to Scala.js 1.0. For type supports, I wonder if we could use the type function pattern matching features to extract out the tagType (just throwing out an idea, haven't written too much Scala 3 code myself yet). This doesn't seem like a use case that's too unreasonable for Scala 3.

@povder povder mentioned this pull request Sep 4, 2021
@shadaj
Copy link
Owner

shadaj commented Oct 9, 2021

@oleg-py it seems like there's a fair bit of demand for partial Scala 3 support, so let's see if we can get this merged! I think the current set of supported features should be fine (supports and @react have sufficient workarounds).

Looks like right now CI is failing due to style checks?

@shadaj
Copy link
Owner

shadaj commented Oct 13, 2021

Looks like native is failing (and vr will probably fail too). Let's disable those on Scala 3 for now, and just get core and web working since that's what most folks depend on.

@shadaj
Copy link
Owner

shadaj commented Oct 18, 2021

It's great that the build is passing! I think it's reasonable to drop Scala.js 0.6 support now, since the vast majority of the ecosystem has moved away from 0.6. Will look through the changes in more detail later this week!

@@ -2,7 +2,7 @@ enablePlugins(ScalaJSPlugin)

name := "slinky-history"

libraryDependencies += "org.scala-js" %%% "scalajs-dom" % "1.1.0"
libraryDependencies += ("org.scala-js" %%% "scalajs-dom" % "1.1.0").cross(CrossVersion.for3Use2_13)
Copy link
Contributor

@armanbilge armanbilge Oct 19, 2021

Choose a reason for hiding this comment

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

Suggested change
libraryDependencies += ("org.scala-js" %%% "scalajs-dom" % "1.1.0").cross(CrossVersion.for3Use2_13)
libraryDependencies += "org.scala-js" %%% "scalajs-dom" % "2.0.0"

We have an RC out with proper Scala 3 support, v2 final following shortly.
#511
scala-js/scala-js-dom#595

Copy link
Contributor

Choose a reason for hiding this comment

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

@shadaj
Copy link
Owner

shadaj commented Oct 23, 2021

Couple of nits, but looks great overall!

@oleg-py oleg-py changed the title WIP: Scala 3 support Scala 3 support Oct 27, 2021
@oleg-py oleg-py marked this pull request as ready for review October 27, 2021 08:16
@oleg-py oleg-py requested a review from shadaj October 27, 2021 08:16
Copy link
Owner

@shadaj shadaj left a comment

Choose a reason for hiding this comment

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

Code looks great! One final ask (sorry for forgetting this in the previous review): could you add this to the changelog? Probably can also include a few sub bulletpoints explaining that @react isn't supported (and any other Reader/Writer limitations) and also that Scala.js 0.6.x isn't supported anymore.

Co-authored-by: Arman Bilge <[email protected]>
@oleg-py oleg-py requested a review from shadaj October 29, 2021 07:04
Copy link
Owner

@shadaj shadaj left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the amazing work @oleg-py and @Asamsig!

@shadaj shadaj merged commit e8571ec into shadaj:master Nov 1, 2021
@shadaj shadaj added this to the v0.7.0 milestone Nov 5, 2021
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.

4 participants