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.

Sign up for GitHub

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

Jump to bottom

Refactor ScalaJSApi to insulate worker API and User facing API #1851

Merged

lefou merged 19 commits into com-lihaoyi:main from lolgab:scala-js-revisit-worker-api

May 3, 2022

Member

lolgab commented Apr 27, 2022 •

edited

Loading

Motivation

We want to support the latest Scala.js features without breaking the support for older versions.
In #1714 I migrated to the new Scala.js API which generated a wrong filename
(main.js instead of out.js) in case the build contained multiple modules.
It broke some people tests that were assuming a out.js file being created where the real file was a different one.

Approach

Since workers are internal API, this PR takes a step further in insulating them (using the same approach adopted in mill-mima https://github.com/lolgab/mill-mima/pull/42/files) by duplicating its model classes in two sets:

the user facing APIs need to be stable and are serialized to json with upickle
the worker facing APIs need to not depend on mill internals and are unstable/internal.

Now the worker is responsible to convert between these two APIs.
The user facing API was moved into the main artifact, while the new internal worker-api was moved to a new artifact which is not checked by mima.

New Features

This PR introduces two new targets that deprecate the main fastOpt and fullOpt:
fastLinkJS and fullLinkJS.
These two targets return a Report which is an adaptation of the Scala.js API Report

These two targets are polyfilled for Scala.js 0.6 - 1.2 by generating the new output from the old API output.
This is needed to make test work consistently on all versions since it's hard in Mill to run different targets depending on the value of a target like:

val task = if(scalaJSVersion() > "1.2") fastLinkJSTest else fastOptTest

Changes

Introduce Report adapted from ScalaJS linker API
Introduce fastLinkJS and fastOptJS which return Report
Make test use fastLinkJSTest
Make Scala.js 1.2- and 0.6 always link in legacy mode
Pass legacy boolean parameter to Worker implementation
to switch between Directory API and File (legacy) API
deprecate old implementation

lolgab marked this pull request as ready for review

April 27, 2022 13:16

lolgab requested a review from lefou

April 27, 2022 13:16

This was referenced Apr 27, 2022

Snake demo does not work with Mill 0.10.2 PurpleKingdomGames/indigo#355

Closed

Recent versions of mill break builds. VizierDB/vizier-scala#174

Closed

lolgab added 7 commits

April 30, 2022 14:35


          Refactor ScalaJSApi to insulate worker API and User facing API

f43fe42

- Introduce `fastLinkJS` and `fastOptJS` which return Report
- Make test use `fastLinkJSTest`
- Make Scala.js 1.2- and 0.6 always link in legacy mode
- Pass `legacy` boolean parameter to Worker implementation
  to switch between Directory API and File (legacy) API
- deprecate old implementation


          Deprecate OptimizeMode

5f56f05


          Call worker as DeprecatedScalaJSWorker

b0d2d5d


          Revert Scala version change in test

3828f4e


          Add tests with fastLinkJS and fullLinkJS


          Make linkJs method private[scalajslib]

f4f488a


          Fix deprecations

1e6d152

lolgab force-pushed the scala-js-revisit-worker-api branch from 9e454b4 to 1e6d152 Compare

April 30, 2022 12:36

Member Author

lolgab commented Apr 30, 2022

Rebased to fix conflicts after #1853 was merged.


          Fix error from bad rebase

399b7fb

lolgab commented

View reviewed changes

build.sc

@@ @@ -533,7 +533,7 @@ object scalalib extends MillModule { @@
               object scalajslib extends MillModule {
-                override def moduleDeps = Seq(scalalib, scalajslib.api)
+                override def moduleDeps = Seq(scalalib, scalajslib.`worker-api`)

Member Author

lolgab Apr 30, 2022

Moved the code of api inside scalajslib and created a new internal module called worker-api.

lolgab commented

View reviewed changes

build.sc

@@ @@ -575,13 +575,12 @@ object scalajslib extends MillModule { @@
                 override def generatedSources: Target[Seq[PathRef]] = Seq(generatedBuildInfo())
-                object api extends MillApiModule {
-                  override def moduleDeps = Seq(main.api)

Member Author

lolgab Apr 30, 2022

Having a completely separated worker module has the side effect that now the Scala.js worker doesn't depend on Mill classes at all! :)

lolgab commented

View reviewed changes

scalajslib/src/mill/scalajslib/ScalaJSModule.scala

                     testBridgeInit,
-                    mode == FullOpt,
+                    optimize,

Member Author

lolgab Apr 30, 2022

Since FullOpt and FastOpt were used only internally I deprecated them now.
We can delete them when we release Mill 0.11

lolgab commented

View reviewed changes

scalajslib/src/mill/scalajslib/ScalaJSModule.scala Outdated

+                    toolsClasspath = scalaJSToolsClasspath(),
+                    runClasspath = scalaJSTestDeps() ++ runClasspath(),
+                    mainClass = None,
+                    legacy = true,

Member Author

lolgab Apr 30, 2022

I maintain the old behavior in the legacy methods.

Member

lefou May 3, 2022

If we use forceOutJs instead of legacy, it would be more obvious what exactly we enable here.

lolgab commented

View reviewed changes

scalajslib/src/mill/scalajslib/ScalaJSModule.scala Outdated

Comment on lines 86 to 92

+                def fastOpt: Target[PathRef] = T {
+                  linkTask(optimize = false, legacy = true)().publicModules.head.jsFile
+                }
+                def fullOpt: Target[PathRef] = T {
+                  linkTask(optimize = true, legacy = true)().publicModules.head.jsFile
+                }

Member Author

lolgab Apr 30, 2022

Didn't deprecate these two methods yet because I want to see if the new methods cover all people use cases.
I will deprecate them in a future PR.
I made fastLinkJS and fullLinkJS work with Scala.js 1.2- so we can safely delete them in Mill 0.11 without dropping support for Scala.js 0.6

lolgab commented

View reviewed changes

scalajslib/src/mill/scalajslib/ScalaJSModule.scala

+                  ).map { report => report.publicModules.head.jsFile }
+                }
+                def fastLinkJSTest: Target[Report] = T {

Member Author

lolgab Apr 30, 2022

This is the task tests will use from now on.

lolgab commented

View reviewed changes

scalajslib/src/mill/scalajslib/ScalaJSWorkerApi.scala Outdated Show resolved Hide resolved

lefou reviewed

View reviewed changes

Member

lefou left a comment

I'm a bit overwhelmed by this PR. I already added some comments, but need a second round to really understand which worker does what, and how the deprecated workers work. 🤪

scalajslib/src/mill/scalajslib/ScalaJSModule.scala Outdated Show resolved Hide resolved

scalajslib/src/mill/scalajslib/ScalaJSModule.scala Outdated

+                  linkTask(optimize = true, legacy = true)().publicModules.head.jsFile
+                }
+                private def linkTask(optimize: Boolean, legacy: Boolean): Task[Report] = T.task {

Member

lefou May 1, 2022 •

edited

Loading

Can we have a more descriptive name than legacy? E.g. if this is indicating pre-1.2 versions, we might just name the parameter that way.

Member Author

lolgab May 1, 2022

Actually legacy means "Use the PathOutputFile linking mode instead of PathOutputDirectory" which is the one used by the fastOptJS (which is the command that Scala.js 1.3 deprecated).

So in Scala.js 1.2- it always links using PathOutputFile while in Scala.js 1.3+ the legacy flag decides if you want to use the output file or the directory.

Member

lefou May 2, 2022

What about returnPathIsFile?

Member Author

lolgab May 2, 2022

Since we polyfill the new API we return a Report even in legacy mode with the same structure.
I think legacy is a good enough name. Also it is used only in private apis so users don't need to know about it.

Member

lefou May 2, 2022 •

edited

Loading

To me the name legacy is as good as foo. The switch clearly does something, otherwise we could just leave it out. Even when this is our internal API, we need to maintain it. If I have issues now, how much fun will have my successor later?

What about forceOutJs? We automatically use out.js for older Scala.js versions, but when we set this to true, we explicitly want to refer to a out.js, even in newer versions.

scalajslib/src/mill/scalajslib/ScalaJSWorkerApi.scala Outdated Show resolved Hide resolved

scalajslib/src/mill/scalajslib/ScalaJSWorkerApi.scala Outdated Show resolved Hide resolved

scalajslib/src/mill/scalajslib/api/ScalaJSApi.scala Outdated Show resolved Hide resolved

lolgab and others added 2 commits

May 1, 2022 18:12


          Fix typo in deprecation message

f545cae

Co-authored-by: Tobias Roeser <le.petit.fou@web.de>


          Move Report to a separate file

lolgab force-pushed the scala-js-revisit-worker-api branch from 121d568 to 1170455 Compare

May 1, 2022 16:44


          Remove redundant protected modifier

1682c5c

lolgab requested a review from lefou

May 1, 2022 18:54

lefou mentioned this pull request

Scalajs rework hints lolgab/mill#1

Merged

lefou added 2 commits

May 2, 2022 13:17


          Suggested change to old ScalaJSWorker

25d4aec


          Made bridgeWorker accessible internally and use it

e52d006

lefou and others added 3 commits

May 2, 2022 13:17


          Ugly attempt to fix binary comaptiblity without compromising proper s…

62158d3

…tate


          Avoid failing in old code in deprecated ScalaJSWorker

a764180


          Avoid using var in ScalaJSWorker

bef52e6

Member Author

lolgab commented May 2, 2022

@lefou Merged your PR and tweaked the code a bit more so we don't need to fail in the old code calling the parameterless constructor to not put an extra burden on the users that are updating.

lolgab added 2 commits

May 2, 2022 13:39


          Fix version in deprecation notice

6ba738b


          Fix since in deprecation notice

7c5f683

lefou reviewed

View reviewed changes

Member

lefou left a comment

Looks good to me to merge. I still think, we do ourselves a favor, when we rename the legacy parameter to something like forceOutJs, to make it more clear (without additional documentation).

Member Author

lolgab commented May 3, 2022

legacy is the same name Scala.js uses in its codebase and error messages: https://github.com/scala-js/scala-js/blob/0708917912938714d52be1426364f78a3d1fd269/linker-interface/shared/src/main/scala/org/scalajs/linker/interface/Linker.scala#L53
https://github.com/scala-js/scala-js/search?q=legacy
But if you really think we should rename feel free to commit directly on the branch.
It is ready to merge for me as well 🙂🎉

Member

lefou commented May 3, 2022

legacy is the same name Scala.js uses in its codebase and error messages: https://github.com/scala-js/scala-js/blob/0708917912938714d52be1426364f78a3d1fd269/linker-interface/shared/src/main/scala/org/scalajs/linker/interface/Linker.scala#L53 https://github.com/scala-js/scala-js/search?q=legacy But if you really think we should rename feel free to commit directly on the branch. It is ready to merge for me as well 🙂🎉

Fair enough. I'll edit and merge, as I think, in the quoted link the word is used with the meaning: deprecated or antiquated.

This is my whole point. The meaning of "legacy" will change over time, and I'm pretty sure we will see other "legacy" changes in Scala.js in the future. That's why I'm nitpicking here.

Thank you for improving Scala.js support!

lolgab requested a review from lefou

May 3, 2022 08:37


          Renamed parameter legacy to forceOutJs

f34e4a3

lefou merged commit e038bbd into com-lihaoyi:main

lefou added this to the after 0.10.3 milestone

lolgab deleted the scala-js-revisit-worker-api branch

May 3, 2022 13:42

lefou mentioned this pull request

Experimental scalajs module splitting support #1642

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet