merge

5 files changed, 111 insertions, 116 deletions

diff --git a/contrib/twirllib/src/mill/twirllib/TwirlModule.scala b/contrib/twirllib/src/mill/twirllib/TwirlModule.scala
index 2df70a1f..8f735f3a 100644
--- a/contrib/twirllib/src/mill/twirllib/TwirlModule.scala
+++ b/contrib/twirllib/src/mill/twirllib/TwirlModule.scala
@@ -33,8 +33,7 @@ trait TwirlModule extends mill.Module {
     )
   }
 
-  // REMIND currently it's not possible to override these default settings
-  private def twirlAdditionalImports: Seq[String] = Nil
+  def twirlAdditionalImports: Seq[String] = Nil
 
   private def twirlConstructorAnnotations: Seq[String] = Nil
 
diff --git a/contrib/twirllib/src/mill/twirllib/TwirlWorker.scala b/contrib/twirllib/src/mill/twirllib/TwirlWorker.scala
index ce8175a6..b8fe9c66 100644
--- a/contrib/twirllib/src/mill/twirllib/TwirlWorker.scala
+++ b/contrib/twirllib/src/mill/twirllib/TwirlWorker.scala
@@ -20,21 +20,43 @@ class TwirlWorker {
       case Some((sig, instance)) if sig == classloaderSig => instance
       case _ =>
         val cl = new URLClassLoader(twirlClasspath.map(_.toIO.toURI.toURL).toArray, null)
-        val twirlCompilerClass = cl.loadClass("play.twirl.compiler.TwirlCompiler")
+
+        // Switched to using the java api because of the hack-ish thing going on later.
+        //
+        // * we'll need to construct a collection of additional imports
+        // * it will need to consider the defaults
+        // * and add the user-provided additional imports
+        // * the default collection in scala api is a Seq[String]
+        // * but it is defined in a different classloader (namely in cl)
+        // * so we can not construct our own Seq and pass it to the method - it will be from our classloader, and not compatible
+        // * the java api has a Collection as the type for this param, for which it is much more doable to append things to it using reflection
+        //
+        // NOTE: I tried creating the cl classloader passing the current classloader as the parent:
+        //   val cl = new URLClassLoader(twirlClasspath.map(_.toIO.toURI.toURL).toArray, getClass.getClassLoader)
+        // in that case it was possible to cast the default to a Seq[String], construct our own Seq[String], and pass it to the method invoke- it was compatible.
+        // And the tests passed. But when run in a different mill project, I was getting exceptions like this:
+        // scala.reflect.internal.MissingRequirementError: object scala in compiler mirror not found.
+
+        val twirlCompilerClass = cl.loadClass("play.japi.twirl.compiler.TwirlCompiler")
+
+        // this one is only to get the codec: Codec parameter default value
+        val twirlScalaCompilerClass = cl.loadClass("play.twirl.compiler.TwirlCompiler")
+
         val compileMethod = twirlCompilerClass.getMethod("compile",
           classOf[java.io.File],
           classOf[java.io.File],
           classOf[java.io.File],
           classOf[java.lang.String],
-          cl.loadClass("scala.collection.Seq"),
-          cl.loadClass("scala.collection.Seq"),
+          cl.loadClass("java.util.Collection"),
+          cl.loadClass("java.util.List"),
           cl.loadClass("scala.io.Codec"),
           classOf[Boolean])
 
-        val defaultAdditionalImportsMethod = twirlCompilerClass.getMethod("compile$default$5")
-        val defaultConstructorAnnotationsMethod = twirlCompilerClass.getMethod("compile$default$6")
-        val defaultCodecMethod = twirlCompilerClass.getMethod("compile$default$7")
-        val defaultFlagMethod = twirlCompilerClass.getMethod("compile$default$8")
+        val arrayListClass = cl.loadClass("java.util.ArrayList")
+        val hashSetClass = cl.loadClass("java.util.HashSet")
+
+        val defaultAdditionalImportsMethod = twirlCompilerClass.getField("DEFAULT_IMPORTS")
+        val defaultCodecMethod = twirlScalaCompilerClass.getMethod("compile$default$7")
 
         val instance = new TwirlWorkerApi {
           override def compileTwirl(source: File,
@@ -45,14 +67,28 @@ class TwirlWorker {
                                     constructorAnnotations: Seq[String],
                                     codec: Codec,
                                     inclusiveDot: Boolean) {
+            val defaultAdditionalImports = defaultAdditionalImportsMethod.get(null) // unmodifiable collection
+            // copying it into a modifiable hash set and adding all additional imports
+            val allAdditionalImports =
+              hashSetClass
+                .getConstructor(cl.loadClass("java.util.Collection"))
+                .newInstance(defaultAdditionalImports)
+                .asInstanceOf[Object]
+            val hashSetAddMethod =
+              allAdditionalImports
+                .getClass
+                .getMethod("add", classOf[Object])
+            additionalImports.foreach(hashSetAddMethod.invoke(allAdditionalImports, _))
+
             val o = compileMethod.invoke(null, source,
               sourceDirectory,
               generatedDirectory,
               formatterType,
-              defaultAdditionalImportsMethod.invoke(null),
-              defaultConstructorAnnotationsMethod.invoke(null),
+              allAdditionalImports,
+              arrayListClass.newInstance().asInstanceOf[Object], // empty list seems to be the default
               defaultCodecMethod.invoke(null),
-              defaultFlagMethod.invoke(null))
+              Boolean.box(false)
+            )
           }
         }
         twirlInstanceCache = Some((classloaderSig, instance))
@@ -89,7 +125,7 @@ class TwirlWorker {
     sourceDirectories.foreach(compileTwirlDir)
 
     val zincFile = ctx.dest / 'zinc
-    val classesDir = ctx.dest / 'html
+    val classesDir = ctx.dest
 
     mill.eval.Result.Success(CompilationResult(zincFile, PathRef(classesDir)))
   }
diff --git a/contrib/twirllib/test/resources/hello-world/core/views/wrapper.scala.html b/contrib/twirllib/test/resources/hello-world/core/views/wrapper.scala.html
new file mode 100644
index 00000000..af1f5d8e
--- /dev/null
+++ b/contrib/twirllib/test/resources/hello-world/core/views/wrapper.scala.html
@@ -0,0 +1,5 @@
+@(content: Html)
+
+@defining("test") { className =>
+    <div class="@className">@content</div>
+}
\ No newline at end of file
diff --git a/contrib/twirllib/test/src/mill/twirllib/HelloWorldTests.scala b/contrib/twirllib/test/src/mill/twirllib/HelloWorldTests.scala
index f14a607b..71f25ff4 100644
--- a/contrib/twirllib/test/src/mill/twirllib/HelloWorldTests.scala
+++ b/contrib/twirllib/test/src/mill/twirllib/HelloWorldTests.scala
@@ -4,6 +4,8 @@ import mill.util.{TestEvaluator, TestUtil}
 import utest.framework.TestPath
 import utest.{TestSuite, Tests, assert, _}
 
+import scala.io.Codec
+
 object HelloWorldTests extends TestSuite {
 
   trait HelloBase extends TestUtil.BaseModule {
@@ -12,6 +14,7 @@ object HelloWorldTests extends TestSuite {
 
   trait HelloWorldModule extends mill.twirllib.TwirlModule {
     def twirlVersion = "1.0.0"
+    override def twirlAdditionalImports: Seq[String] = additionalImports
   }
 
   object HelloWorld extends HelloBase {
@@ -23,9 +26,10 @@ object HelloWorldTests extends TestSuite {
 
   val resourcePath: os.Path = os.pwd / 'contrib / 'twirllib / 'test / 'resources / "hello-world"
 
-  def workspaceTest[T](m: TestUtil.BaseModule, resourcePath: os.Path = resourcePath)
-                      (t: TestEvaluator => T)
-                      (implicit tp: TestPath): T = {
+  def workspaceTest[T](
+      m: TestUtil.BaseModule,
+      resourcePath: os.Path = resourcePath
+  )(t: TestEvaluator => T)(implicit tp: TestPath): T = {
     val eval = new TestEvaluator(m)
     os.remove.all(m.millSourcePath)
     os.remove.all(eval.outPath)
@@ -35,14 +39,30 @@ object HelloWorldTests extends TestSuite {
   }
 
   def compileClassfiles: Seq[os.RelPath] = Seq[os.RelPath](
-    "hello.template.scala"
+    "hello.template.scala",
+    "wrapper.template.scala"
+  )
+
+  def expectedDefaultImports: Seq[String] = Seq(
+    "import _root_.play.twirl.api.TwirlFeatureImports._",
+    "import _root_.play.twirl.api.TwirlHelperImports._",
+    "import _root_.play.twirl.api.Html",
+    "import _root_.play.twirl.api.JavaScript",
+    "import _root_.play.twirl.api.Txt",
+    "import _root_.play.twirl.api.Xml"
+  )
+
+  def additionalImports: Seq[String] = Seq(
+    "mill.twirl.test.AdditionalImport1._",
+    "mill.twirl.test.AdditionalImport2._"
   )
 
   def tests: Tests = Tests {
     'twirlVersion - {
 
       'fromBuild - workspaceTest(HelloWorld) { eval =>
-        val Right((result, evalCount)) = eval.apply(HelloWorld.core.twirlVersion)
+        val Right((result, evalCount)) =
+          eval.apply(HelloWorld.core.twirlVersion)
 
         assert(
           result == "1.3.15",
@@ -53,20 +73,26 @@ object HelloWorldTests extends TestSuite {
     'compileTwirl - workspaceTest(HelloWorld) { eval =>
       val Right((result, evalCount)) = eval.apply(HelloWorld.core.compileTwirl)
 
-      val outputFiles = os.walk(result.classes.path)
+      val outputFiles = os.walk(result.classes.path).filter(_.last.endsWith(".scala"))
       val expectedClassfiles = compileClassfiles.map(
         eval.outPath / 'core / 'compileTwirl / 'dest / 'html / _
       )
+
       assert(
-        result.classes.path == eval.outPath / 'core / 'compileTwirl / 'dest / 'html,
+        result.classes.path == eval.outPath / 'core / 'compileTwirl / 'dest,
         outputFiles.nonEmpty,
         outputFiles.forall(expectedClassfiles.contains),
-        outputFiles.size == 1,
-        evalCount > 0
+        outputFiles.size == 2,
+        evalCount > 0,
+        outputFiles.forall { p =>
+          val lines = os.read.lines(p).map(_.trim)
+          (expectedDefaultImports ++ additionalImports.map(s => s"import $s")).forall(lines.contains)
+        }
       )
 
       // don't recompile if nothing changed
-      val Right((_, unchangedEvalCount)) = eval.apply(HelloWorld.core.compileTwirl)
+      val Right((_, unchangedEvalCount)) =
+        eval.apply(HelloWorld.core.compileTwirl)
 
       assert(unchangedEvalCount == 0)
     }
diff --git a/docs/pages/9 - Contrib Modules.md b/docs/pages/9 - Contrib Modules.md
index e071d59c..4d811cde 100644
--- a/docs/pages/9 - Contrib Modules.md
+++ b/docs/pages/9 - Contrib Modules.md
@@ -161,14 +161,16 @@ Also note that twirl templates get compiled into scala code, so you also need to
 ```scala
 import $ivy.`com.lihaoyi::mill-contrib-twirllib:0.3.2`,  mill.twirllib._
 object app extends ScalaModule with TwirlModule {
-
+// ...
 } 
 ``` 
 
 #### Configuration options
 
-* ` def twirlVersion: T[String]` (mandatory) - the version of the twirl compiler to use, like "1.3.15"
-
+* `def twirlVersion: T[String]` (mandatory) - the version of the twirl compiler to use, like "1.3.15"
+* `def twirlAdditionalImports: Seq[String] = Nil` - the additional imports that will be added by twirl compiler to the top
+  of all templates
+  
 #### Details
 
 The following filesystem layout is expected:
@@ -197,107 +199,34 @@ object app extends ScalaModule with TwirlModule {
 }
 ``` 
 
-#### Caveats
-
-There is a couple of caveats, to be aware of, as of now (in `v0.3.2`).
-
-##### Packages 
-First, if you structure your twirl templates into packages, like this:
-```text
-build.sc
-app/
-    src/hello/
-        Main.scala
-    views/
-        hello/
-            another/ 
-                view1.scala.html
-                view2.scala.html
-```
-
-the generated sources in the `out` directory will look like this:
-```text
-build.sc
-out/app/compileTwirl/dest/
-    hello/
-      another/
-        html/
-          view1.template.scala
-          view2.template.scala
-```
-
-Looking at the `mill show app.compileTwirl` in this setup shows this:
-```
-{
-    ...
-    "classes": "ref: ... : .../out/app/compileTwirl/dest/html"
-}
-```
-
-Basically it means that currently `TwirlModule` expects all templates to be html and with no packages.
-So adding this directly to the generated sources will not exactly work as expected (as there might not even be a `out/app/compileTwirl/dest/html` directory
-at all, unless you have templates in the default package).
-
-The workaround is simple, though:
+To add additional imports to all of the twirl templates:
 ```scala
 object app extends ScalaModule with TwirlModule {
   def twirlVersion = "1.3.15"
-  override def generatedSources = T{
-    val classes = compileTwirl().classes
-    Seq(classes.copy(path = classes.path / os.up)) // we just move one dir up
-  }
+  override def twirlAdditionalImports = Seq("my.additional.stuff._", "my.other.stuff._")
+  def generatedSources = T{ Seq(compileTwirl().classes) }
 }
 ``` 
 
-This should cover the problem with templates under packages, and also should make other-than-html 
-templates available as well.
-
-##### Default imports
-
-Another problem is with some default imports that the twirl sbt plugin assumes, but it seems not to work with `TwirlModule`.
-
-If you reference `Html` in your templates, like
-
+as the result all templates will get this line at the top:
 ```scala
-// wrapper.scala.html
-@(content: Html)
-<div class="wrapper">
-  @content
-</div>
+@import "my.additional.stuff._"
+@import "my.other.stuff._"
 ```
 
-the template will not compile. You'll need to add this import:
-```
-@import play.twirl.api._
-```
-
-in the template that uses twirl classes.
-
-Another one is `@defining`, which might be used like this:
-```
-@defining({
-  val calculatedClass = {
-    // do some calculations here
-  }
-  calculatedClass
-}) { calculatedClass =>
-    <div class="@calculatedClass">stuff 1</div>
-    <div class="@calculatedClass">stuff 2</div>
-}
-```
-
-You'll need this import:
+Besides that, twirl compiler has default imports, at the moment these:
 ```scala
-@import play.twirl.api.TwirlFeatureImports._
-```
-
-At some point `TwirlModule` might get support for the additional "default" imports, which will make this much easier, 
-but right now it is unimplemented
+Seq(
+    "_root_.play.twirl.api.TwirlFeatureImports._",
+    "_root_.play.twirl.api.TwirlHelperImports._",
+    "_root_.play.twirl.api.Html",
+    "_root_.play.twirl.api.JavaScript",
+    "_root_.play.twirl.api.Txt",
+    "_root_.play.twirl.api.Xml"
+)
+``` 
 
-```scala
-  // REMIND currently it's not possible to override these default settings
-  private def twirlAdditionalImports: Seq[String] = Nil
-```
+These imports will always be added to every template.  You don't need to list them if you override `twirlAdditionalImports`.
 
 #### Example
 There's an [example project](https://github.com/lihaoyi/cask/tree/master/example/twirl)