Added support for Scala JS for the main scripts #930

dylanowen · 2018-07-17T06:44:03Z

Not Ready To Merge

This is a partial implementation of using Scala JS for our site's scripts instead of Vanilla JS.

I'm opening this pull request mainly to get feedback on the direction and to see if it's worth completing. I'm totally fine with tossing this code if we don't think it makes sense for the project.

Should we actually do this?
This is a large change with multiple moving pieces. Should we really be changing something that isn't broken?

Other Concerns

Maintainability: If our maintainers are relying on bundler vs the docker-compose scripts we'll be breaking their workflow.
I'm hoping this makes the code more accessible to scala-lang maintainers but I don't have any insights on if JS actually makes mores sense.
This makes our builds more complex.

Missing Pieces

Compiling the Scala in our build (do we use travis, jenkins, or both?)
a. sbt fullOptJS I'm using the closure compiler to minify the output but we can turn that off to make prod debugging easier

Anyway let me know if I should continue and if there is any feedback on my implementation.

dylanowen · 2018-07-17T06:45:27Z

docker-compose.yml

+      -   80:4000
+    command: jekyll serve --incremental


I'm assuming that docker-compose is only used for development, but if this isn't the case I could be breaking things here

correct it's optional dev-only stuff

dylanowen · 2018-07-17T06:46:35Z

js-src/functions/src/main/scala/org/scalalang/utils/Logger.scala

+/**
+  * A basic logger for the debugging console
+  */
+object Logger {


I browsed around for a few scala js logging implementation but didn't find one that fit my needs. I'm open to swapping this out for a different project vs my home-built solution

perhaps you could put a comment to that effect at the top of the file, and say briefly what the considerations were

You have two actual uses of this logger. I suggest replacing them by a good old println.

dylanowen · 2018-07-17T06:47:26Z

resources/js/functions.js

@@ -1,6 +1,3 @@
---
---


I believe this isn't needed after #924

SethTisue

My familiarity with Scala.js and front-end stuff in general is limited, but:

Cool! I like it and I lean towards accepting this (once complete). I think being used on scala-lang.org is good bragging rights for Scala.js.

My sense is that is that if we end up having Scala.js-related trouble there will be no shortage of volunteers wanting to help out. And yeah it makes the build more complicated, but I doubt that will affect anyone doing ordinary work on the site.

Interested in hearing other opinions on this. (@heathermiller? @sjrd?)

SethTisue · 2018-07-19T22:35:36Z

docker-compose.yml

+      -   80:4000
+    command: jekyll serve --incremental


correct it's optional dev-only stuff

SethTisue · 2018-07-19T22:36:52Z

js-src/functions/src/main/scala/org/scalalang/utils/Logger.scala

+/**
+  * A basic logger for the debugging console
+  */
+object Logger {


perhaps you could put a comment to that effect at the top of the file, and say briefly what the considerations were

sjrd · 2018-07-20T19:48:15Z

I am skeptical. Even https://www.scala-js.org/ does not use any Scala.js code, and for one good reason: it is a mostly static website, with a couple scripts for isolated functions on the site. Most of the code consists in external libraries. The same applies to scala-lang.org.

Scala.js does not shine on small scripts. You will bloat the download times because the Scala.js code size "tax" will be much larger than the features you are implementing---unlike in larger applications, where Scala.js is appropriate because the tax is dwarfed by the actual codebase of the website.

Just look at the PR stats: +441 for -91. If you're increasing the codebase size by porting to Scala.js, you're likely doing yourself a disservice. One would say that most of it is about build scripts, and that is true. But let us also look at the features themselves. For example, this PR replaces the JavaScript code:

// OS detection
function getOS() {
  var osname = "linux";
  if (navigator.appVersion.indexOf("Win") != -1) osname = "windows";
  if (navigator.appVersion.indexOf("Mac") != -1) osname = "osx";
  if (navigator.appVersion.indexOf("Linux") != -1) osname = "linux";
  if (navigator.appVersion.indexOf("X11") != -1) osname = "unix";
  return osname;
}

$(document).ready(function() {
    if ($(".main-download").length) {
        var os = getOS();
        var intelliJlink = $("#intellij-" + os).text();
        var sbtLink = $("#sbt-" + os).text();
        var stepOneContent = $("#stepOne-" + os).html()
        $("#download-intellij-link").attr("href", intelliJlink);
        $("#download-sbt-link").attr("href", sbtLink);
        $("#download-step-one").html(stepOneContent);
    }
});

by a complex abstraction for the OS in addition to the following Scala.js code:

  private def setupMainDownload(): Unit = JsUtils.findElement(".main-download").foreach(_ => {
    val osLabel: String = OS().label

    val intelliJlink: String = $("#intellij-" + osLabel).text()
    val sbtLink: String = $("#sbt-" + osLabel).text()
    val stepOneContent: String = $("#stepOne-" + osLabel).html()

    $("#download-intellij-link").attr("href", intelliJlink)
    $("#download-sbt-link").attr("href", sbtLink)
    $("#download-step-one").html(stepOneContent)
  })

What have we gained? Virtually nothing. Just a little bit of type-safety, which TypeScript would have also been able to provide, without the Scala.js bloat.

I think the PR was a worthwhile experiment, if only to demonstrate that Scala.js is not the best option for this specific job.

sjrd

Here are a couple suggestions for improvements, in case you decide to go through with this.

sjrd · 2018-07-20T19:49:27Z

js-src/functions/build.sbt

+
+// move our output folder to static
+artifactPath in(Compile, fastOptJS) := baseDirectory.value / ".." / ".." / "resources" / "js" / s"scala-${name.value}.js"
+artifactPath in(Compile, fullOptJS) := baseDirectory.value / ".." / ".." / "resources" / "js" / s"scala-${name.value}.js"


Don't configure fastOptJS and fullOptJS to produce the same target file. It will confuse caches (fullOptJS after fastOptJS will not recreate the file, for example).

Oh good to know. What's the recommended way to setup a build like this? (Where I'd like my html file to be static and to just import the compiled output regardless of compilation target)

I'm not sure. I think it depends on the web server and/or templating mechanisms that one use. The folks at https://gitter.im/scala-js/scala-js or https://stackoverflow.com/questions/tagged/scala.js might have more experience with that than I do.

sjrd · 2018-07-20T19:49:44Z

js-src/functions/build.sbt

@@ -0,0 +1,18 @@
+import org.scalajs.core.tools.linker.backend.OutputMode


This import is unused.

sjrd · 2018-07-20T19:50:45Z

js-src/functions/build.sbt

+// move our output folder to static
+artifactPath in(Compile, fastOptJS) := baseDirectory.value / ".." / ".." / "resources" / "js" / s"scala-${name.value}.js"
+artifactPath in(Compile, fullOptJS) := baseDirectory.value / ".." / ".." / "resources" / "js" / s"scala-${name.value}.js"
+scalaJSOptimizerOptions in (Compile, fullOptJS) ~= { _.withUseClosureCompiler(true) }


This setting is redundant, as it is the default.

sjrd · 2018-07-20T19:51:49Z

js-src/functions/build.sbt

+scalaJSOptimizerOptions in (Compile, fullOptJS) ~= { _.withUseClosureCompiler(true) }
+
+scalaJSUseMainModuleInitializer := true
+emitSourceMaps := false


Prefer the more future-proof

scalaJSLinkerConfig ~= { _.withSourceMap(false) }

sjrd · 2018-07-20T19:53:50Z

js-src/functions/src/main/scala/org/scalalang/utils/Logger.scala

+/**
+  * A basic logger for the debugging console
+  */
+object Logger {


You have two actual uses of this logger. I suggest replacing them by a good old println.

sjrd · 2018-07-20T19:56:06Z

js-src/functions/src/main/scala/org/scalalang/utils/OS.scala

+        else {
+          last
+        }
+      })


More simply:

val os = Array(Windows, Mac, Linux, Unix) .find(os => appVersion.contains(os.navigator)) .getOrElse(default)

sjrd · 2018-07-20T19:57:38Z

js-src/functions/src/main/scala/org/scalalang/utils/OS.scala

+  */
+sealed trait OS {
+  val navigator: String
+  val label: String


In order to cut down on the boilerplate, consider declaring OS as

sealed abstract class OS(val navigator: String, val label: String)

so that the instances are one-liners:

case object Windows extends OS("Win", "Windows")

dylanowen · 2018-07-21T20:57:02Z

Thanks for the feedback and detailed comments @SethTisue and @sjrd 😄

While this was a fun exercise I'm inclined to agree with @sjrd's analysis: we're better off sticking with vanilla JS.

I'm closing out this PR but I've updated my code with the feedback so if we ever decide Scala.js makes sense we'll know where to find it.

jvican · 2018-07-22T08:32:48Z

Thanks for your work Dylan 👍

Just in case you're thinking of playing around with Scala.js more, you can have a look at scala/docs.scala-lang#1107.

dylanowen added 5 commits July 14, 2018 15:49

initial scala-js implementation

03171a7

cleaned up things and added the tooltips

7a868f8

cleanup

94bec68

first pass at a local incremental build

27a9cfb

tiny cleanup

28dffda

dylanowen commented Jul 17, 2018

View reviewed changes

resources/js/functions.js

@@ -1,6 +1,3 @@

---

---

Copy link

Contributor Author

dylanowen Jul 17, 2018

Choose a reason for hiding this comment

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

I believe this isn't needed after #924

setInfo logging

61f7165

SethTisue reviewed Jul 19, 2018

View reviewed changes

sjrd reviewed Jul 20, 2018

View reviewed changes

updated with review comments

12ed633

dylanowen closed this Jul 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for Scala JS for the main scripts #930

Added support for Scala JS for the main scripts #930

dylanowen commented Jul 17, 2018 •

edited

Loading

dylanowen Jul 17, 2018

SethTisue Jul 19, 2018

dylanowen Jul 17, 2018

SethTisue Jul 19, 2018

sjrd Jul 20, 2018

dylanowen Jul 17, 2018

SethTisue left a comment •

edited

Loading

SethTisue Jul 19, 2018

SethTisue Jul 19, 2018

sjrd commented Jul 20, 2018

sjrd left a comment

sjrd Jul 20, 2018

dylanowen Jul 21, 2018

sjrd Jul 21, 2018

sjrd Jul 20, 2018

sjrd Jul 20, 2018

sjrd Jul 20, 2018

sjrd Jul 20, 2018

sjrd Jul 20, 2018

sjrd Jul 20, 2018

dylanowen commented Jul 21, 2018

jvican commented Jul 22, 2018

		@@ -0,0 +1,18 @@
		import org.scalajs.core.tools.linker.backend.OutputMode

Added support for Scala JS for the main scripts #930

Added support for Scala JS for the main scripts #930

Conversation

dylanowen commented Jul 17, 2018 • edited Loading

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

SethTisue left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjrd commented Jul 20, 2018

sjrd left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanowen commented Jul 21, 2018

jvican commented Jul 22, 2018

dylanowen commented Jul 17, 2018 •

edited

Loading

SethTisue left a comment •

edited

Loading