Support Scala.js 1.5.0 #33

clutroth · 2021-03-22T13:44:52Z

I added scalajs 1.5.0 support. A allowed using deprecated "file as output" with warring, and ported to new API. WDYT?

gzm0

Thank you for opening this PR!

gzm0 · 2021-03-23T19:01:05Z

.gitignore

 target/
 /pack/
 /cli-test/
+.idea/


We usually don't put IDE specific things into the checked-in .gitignore. Put it in your global .gitignore?

gzm0 · 2021-03-23T19:05:34Z

src/main/scala/org/scalajs/cli/Scalajsld.scala

+    val arity: Int = 1
+    val reads = { (s: String) =>
+      val file = implicitly[scopt.Read[File]].reads(s)
+      if(file.isDirectory){


I'm not sure this is very user friendly. Essentially, if the output directory doesn't exist, instead of failing or creating it, it falls back to the old behavior (potentially creating a file where a directory is expected).

I think another flag that is mutually exclusive with this one is needed. Unfortunately, the obvious candidate -d (same option as in scalac / javac) is already taken. Not sure what a good flag would be.

Ok, so I'll restore exact previous behavior, and introduce new option. IMHO sustaining backward compatibility is more important than one-letter option. I'll introduce output-dir option for directory output, and forbid simultaneous use of -o, and - output-dir . WDYT?

src/main/scala/org/scalajs/cli/Scalajsld.scala

gzm0 · 2021-03-23T19:12:54Z

build.sbt

+  crossScalaVersions := Seq("2.12.13", "2.11.12", "2.13.5"),
  scalaVersion := crossScalaVersions.value.head,
-  scalacOptions ++= Seq("-deprecation", "-feature", "-Xfatal-warnings"),
+  scalacOptions ++= Seq("-deprecation", "-feature"/*, "-Xfatal-warnings" FIMXE: commented because using legacy API */),


We need a better solution for this :) Have a look here (not pretty, but the best way we could find):

https://github.com/scala-js/scala-js/blob/d09ce384d35e3310272e8099dda5f2d3fee8dac1/sbt-plugin/src/main/scala/org/scalajs/sbtplugin/ScalaJSPluginInternal.scala#L265-L292

gzm0 · 2021-03-23T19:14:25Z

scripts/test-cli.sh

+diff got-legacy.run want-legacy.run
+
+mkdir test-output
+scalajsld -s -o test-output -mm Foo.main bin


It might be nice to use SplitStyle.SmallestModules here to make sure the wiring is actually correct.

gzm0 · 2021-03-23T19:14:48Z

scripts/test-cli.sh

 EOF

-diff got.run want.run
+diff got.run want.run


Please put a newline at the end of the file.

gzm0

Mostly minor stuff, only larger point: is --outputDirectory good enough without short version. @sjrd might also have an opinion on this.

src/main/scala/org/scalajs/cli/Scalajsld.scala

gzm0 · 2021-04-03T13:31:09Z

src/main/scala/org/scalajs/cli/Scalajsld.scala

+      opt[ModuleSplitStyle]("moduleSplitStyle")
+        .action { (x, c) => c.copy(moduleSplitStyle = x) }
+        .text("Module splitting strategy " + ModuleSplitStyleRead.All.mkString("(", ", ", ")"))
+      opt[String]("outputPatterns")


call this jsFilePattern? I think for now it is OK to only support this specific type of pattern, but we should reserve the outputPatterns name for that option, if we ever want to introduce it.

gzm0 · 2021-04-03T13:32:03Z

src/main/scala/org/scalajs/cli/Scalajsld.scala

        .text("Don't optimize code")
+      opt[ModuleSplitStyle]("moduleSplitStyle")
+        .action { (x, c) => c.copy(moduleSplitStyle = x) }
+        .text("Module splitting strategy " + ModuleSplitStyleRead.All.mkString("(", ", ", ")"))


Module splitting style (style and strategy really both don't add any semantic value here IMO, but then we should stick with the terminology that already exists).

gzm0 · 2021-04-03T13:32:31Z

src/main/scala/org/scalajs/cli/Scalajsld.scala

      esFeatures: ESFeatures = ESFeatures.Defaults,
      moduleKind: ModuleKind = ModuleKind.NoModule,
+      moduleSplitStyle: ModuleSplitStyle = ModuleSplitStyle.FewestModules,
+      outputPatterns:OutputPatterns = OutputPatterns.Defaults,


Space after :.

gzm0 · 2021-04-03T13:33:57Z

src/main/scala/org/scalajs/cli/Scalajsld.scala

-        .text("Output file of linker (required)")
+        .action { (x, c) => c.copy(output = Some(x)) }
+        .text("Output file of linker (deprecated)")
+      opt[File]("outputDir")


I'm not a fan of not having a short option for a de-facto required flag (it's only not required for deprecated usage).

gzm0 · 2021-04-03T13:46:39Z

src/main/scala/org/scalajs/cli/Scalajsld.scala

+    }
+
+    @deprecated("Deprecate to silence warnings", "never/always")
+    private def deprecatedOutput(file: File) = {


It might be OK to inline this in the method above.

gzm0 · 2021-04-03T13:46:47Z

src/main/scala/org/scalajs/cli/Scalajsld.scala

    }
  }
+
+  private def createOutputFile(options: Options): Option[OutputFile] = {


gzm0 · 2021-04-03T13:46:55Z

src/main/scala/org/scalajs/cli/Scalajsld.scala


    for (options <- parser.parse(args, Options())) {
+      val outputFile = createOutputFile(options)
+        .getOrElse(throw new IllegalStateException("output either outputDir have to be defined"))


gzm0 · 2021-04-03T13:49:05Z

scripts/test-cli.sh

+mkdir test-output
+scalajsld -s --outputDir test-output --moduleSplitStyle SmallestModules --moduleKind CommonJSModule -mm Foo.main bin
+test -s test-output/Foo.js || fail "scalajsld: empty output"
+test -s test-output/Foo.js.map || fail "scalajsld: empty output"


These file names are not specified (or more precisely: the module ID is an implementation detail subject to change). Maybe just assert that there is more than one .js file?

gzm0 · 2021-04-03T13:49:50Z

scripts/test-cli.sh

+test -s test-output/Foo.js || fail "scalajsld: empty output"
+test -s test-output/Foo.js.map || fail "scalajsld: empty output"
+test -s test-output/main.js || fail "scalajsld: empty output"
+test -s test-output/main.js.map || fail "scalajsld: empty source map"


It's maybe a good idea to have more specific error messages here. Otherwise it's hard to understand which part of the output is empty. (n.b. fail will not report the line it failed on, so calling it twice with the same string should be avoided if possible).

sjrd · 2021-04-03T18:06:50Z

Mostly minor stuff, only larger point: is --outputDirectory good enough without short version. @sjrd might also have an opinion on this.

Indeed, this option should have a short name. If you can't find anything meaningful, use something meaningless like -z. ;)

gzm0

Sorry this took so long. I'm struggling to find time for Scala.js.

I suggested a help text for jsFilePattern. Otherwise only codingstyle nits.

If all of this is addressed, feel free to squash everything into a single commit right away.

src/main/scala/org/scalajs/cli/Scalajsld.scala

gzm0 · 2021-05-02T15:25:23Z

src/main/scala/org/scalajs/cli/Scalajsld.scala

+              DeprecatedLinkerAPI().link(linker, irFiles.toList, moduleInitializers, jsFile, logger)
+            case (None, Some(outputDir)) =>
+              linker.link(irFiles, moduleInitializers, PathOutputDirectory(outputDir.toPath), logger)
+            case _ => throw new Exception("Either output or outputDir have to be defined.")


Throw an AssertionError? If we get here: The previous code has failed. So it's not a user error anymore, but a bug in the code.

gzm0 · 2021-05-02T15:27:31Z

src/main/scala/org/scalajs/cli/Scalajsld.scala

+             irFiles: Seq[IRFile],
+             moduleInitializers: Seq[ModuleInitializer],
+             linkerOutputFile: File,
+             logger: Logger): Future[Unit]


The indentation here is not following SJS style: https://github.com/scala-js/scala-js/blob/master/CODINGSTYLE.md#indentation

Either binpack it (as many args per line as possible, 4 spaces indent), or one per line, 2 spaces indented.

I'm not sure, if it's this case. It'd be right for function invocation, but no declaration. I applied rule In general, indentation is 2 spaces, except continuation lines, which are indented 4 spaces. following example org.scalajs.linker.interface.Linker#link

gzm0 · 2021-05-02T15:31:51Z

src/main/scala/org/scalajs/cli/Scalajsld.scala

+        .text("Module splitting strategy " + ModuleSplitStyleRead.All.mkString("(", ", ", ")"))
+      opt[String]("outputPatterns")
+        .action { (x, c) => c.copy(outputPatterns = OutputPatterns.fromJSFile(x)) }
+        .text("Creates output patterns from a JS file pattern.")


How about: "Pattern for JS file names (default: %s.js). Expects a printf-style pattern with a single placeholder for the module ID. A typical use case is changing the file extension, e.g. %.mjs for Node.js modules."?

clutroth · 2021-06-10T13:47:02Z

@gzm0 could we proceed?

gzm0

Sorry, this slipped through the cracks. Thank you for reminding me. Only minor things, really.

gzm0 · 2021-05-09T17:16:48Z

src/main/scala/org/scalajs/cli/Scalajsld.scala

+        .action { (x, c) => c.copy(outputPatterns = OutputPatterns.fromJSFile(x)) }
+        .text("Pattern for JS file names (default: %s.js). " +
+            "Expects a printf-style pattern with a single placeholder for the module ID. " +
+            "A typical use case is changing the file extension, e.g. %.mjs for Node.js modules.")


Use backticks for the examples? (like in my comment, but the markdown version of it)?

gzm0 · 2021-06-12T07:40:10Z

src/main/scala/org/scalajs/cli/Scalajsld.scala

+      checkConfig { c =>
+        if (c.output.isDefined) {
+          reportWarning("using a single file as output (--output) is deprecated since Scala.js 1.3.0." +
+            " Use --outputDirectory instead.")


--outputDir.

gzm0

LGTM. Please squash all the commits and then we can merge this (unless @sjrd has comments / objections).

sjrd · 2021-06-12T16:55:17Z

No objection. ;) Looks good to me.

gzm0 requested changes Mar 23, 2021

View reviewed changes

clutroth requested a review from gzm0 March 25, 2021 13:01

gzm0 requested changes Apr 3, 2021

View reviewed changes

clutroth requested a review from gzm0 April 16, 2021 06:31

gzm0 requested changes May 2, 2021

View reviewed changes

clutroth requested a review from gzm0 May 6, 2021 14:57

gzm0 requested changes Jun 12, 2021

View reviewed changes

clutroth requested a review from gzm0 June 12, 2021 10:57

gzm0 changed the title ~~support scalajs 1.5.0~~ Support Scala.js 1.5.0 Jun 12, 2021

gzm0 reviewed Jun 12, 2021

View reviewed changes

Support Scala.js 1.5.0

d5139c1

clutroth force-pushed the support-150 branch from fe30dac to d5139c1 Compare June 16, 2021 14:24

clutroth requested a review from gzm0 June 16, 2021 14:31

gzm0 approved these changes Jun 16, 2021

View reviewed changes

gzm0 merged commit 86278ac into scala-js:master Jun 16, 2021

Support Scala.js 1.5.0 #33

Support Scala.js 1.5.0 #33

Uh oh!

Conversation

clutroth commented Mar 22, 2021

Uh oh!

gzm0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzm0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjrd commented Apr 3, 2021

Uh oh!

gzm0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clutroth commented Jun 10, 2021

Uh oh!

gzm0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzm0 left a comment

Choose a reason for hiding this comment

Uh oh!

sjrd commented Jun 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone