JS engines supporting node should prefer local installed npm

Instead of using webjars npm

build.sbt

@@ -23,7 +23,7 @@ libraryDependencies ++= Seq(
   "io.apigee.trireme" % "trireme-node10src" % "0.9.4",
 
   // NPM
-  "org.webjars" % "npm" % "5.0.0-2",
+  "org.webjars" % "npm" % "5.0.0-2", // we are currently stuck: https://github.com/webjars/webjars/issues/1926
   "org.webjars" % "webjars-locator-core" % "0.52",
 
   // Test deps

src/main/scala/com/typesafe/sbt/jse/SbtJsEngine.scala

@@ -34,6 +34,8 @@ object JsEngineImport {
     @deprecated("No longer used", "1.3.0")
     val npmTimeout = SettingKey[FiniteDuration]("jse-npm-timeout", "The maximum number amount of time for npm to do its thing.")
     val npmNodeModules = TaskKey[Seq[File]]("jse-npm-node-modules", "Node module files generated by NPM.")
+    val npmPreferSystemInstalledNpm = SettingKey[Boolean]("jse-npm-prefer-system-installed-npm","Prefer detecting and using locally installed NPM when using a local engine that provides Node support")
+    // TODO: Run install, update or ci ?
   }
 
 }
@@ -80,7 +82,9 @@ object SbtJsEngine extends AutoPlugin {
   private lazy val autoDetectNode: Boolean = {
     val nodeExists = Try(Process("node --version").!!).isSuccess
     if (!nodeExists) {
+      println("!!!")
       println("Warning: node.js detection failed, sbt will use the Rhino based Trireme JavaScript engine instead to run JavaScript assets compilation, which in some cases may be orders of magnitude slower than using node.js.")
+      println("!!!")
     }
     nodeExists
   }
@@ -96,10 +100,11 @@ object SbtJsEngine extends AutoPlugin {
       val nodeModulesDirectory = (Plugin / webJarsNodeModulesDirectory).value
       val logger = streams.value.log
       val baseDir = baseDirectory.value
+      val preferSystemInstalledNpm = npmPreferSystemInstalledNpm.value
 
       val runUpdate = FileFunction.cached(cacheDirectory, FilesInfo.hash) { _ =>
         if (npmPackageJson.exists) {
-          val npm = new Npm(engine, nodeModulesDirectory / "npm" / "lib" / "npm.js")
+          val npm = new Npm(engine, nodeModulesDirectory / "npm" / "lib" / "npm.js", preferSystemNpm = preferSystemInstalledNpm )
           val result = npm.update(global = false, Seq("--prefix", baseDir.getPath), logger.info(_), logger.error(_))
           if (result.exitValue != 0) {
             sys.error("Problems with NPM resolution. Aborting build.")
@@ -125,7 +130,8 @@ object SbtJsEngine extends AutoPlugin {
         println(s"Unknown engine type $engineTypeStr for sbt.jse.engineType. Resorting back to the default of $defaultEngineType.")
         defaultEngineType
       }),
-    command := sys.props.get("sbt.jse.command").map(file)
+    command := sys.props.get("sbt.jse.command").map(file),
+    npmPreferSystemInstalledNpm := true,
 
   ) ++ inConfig(Assets)(jsEngineUnscopedSettings) ++ inConfig(TestAssets)(jsEngineUnscopedSettings)
 

src/main/scala/com/typesafe/sbt/jse/engines/LocalEngine.scala

@@ -6,27 +6,7 @@ import scala.collection.JavaConverters._
 import scala.collection.immutable
 import scala.sys.process.{Process, ProcessLogger}
 
-class LocalEngine(stdArgs: immutable.Seq[String], stdEnvironment: Map[String, String], override val isNode: Boolean) extends Engine {
-
-  // This quoting functionality is as recommended per http://bugs.java.com/view_bug.do?bug_id=6511002
-  // The JDK can't change due to its backward compatibility requirements, but we have no such constraint
-  // here. Args should be able to be expressed consistently by the user of our API no matter whether
-  // execution is on Windows or not.
-  private def needsQuoting(s: String): Boolean =
-    if (s.isEmpty) true else s.exists(c => c == ' ' || c == '\t' || c == '\\' || c == '"')
-
-  private def winQuote(s: String): String = {
-    if (!needsQuoting(s)) {
-      s
-    } else {
-      "\"" + s.replaceAll("([\\\\]*)\"", "$1$1\\\\\"").replaceAll("([\\\\]*)\\z", "$1$1") + "\""
-    }
-  }
-
-  private val isWindows: Boolean = System.getProperty("os.name").toLowerCase.contains("win")
-
-  private def prepareArgs(args: immutable.Seq[String]): immutable.Seq[String] =
-    if (isWindows) args.map(winQuote) else args
+class LocalEngine(val stdArgs: immutable.Seq[String], val stdEnvironment: Map[String, String], override val isNode: Boolean) extends Engine {
 
   override def executeJs(source: File, args: immutable.Seq[String], environment: Map[String, String],
     stdOutSink: String => Unit, stdErrSink: String => Unit): JsExecutionResult = {
@@ -35,7 +15,7 @@ class LocalEngine(stdArgs: immutable.Seq[String], stdEnvironment: Map[String, St
     val allEnvironment = stdEnvironment ++ environment
 
 
-    val pb = new ProcessBuilder(prepareArgs(allArgs).asJava)
+    val pb = new ProcessBuilder(LocalEngine.prepareArgs(allArgs).asJava)
     pb.environment().putAll(allEnvironment.asJava)
     JsExecutionResult(Process(pb).!(ProcessLogger(stdOutSink, stdErrSink)))
   }
@@ -56,6 +36,26 @@ object LocalEngine {
     val newNodePath = Option(System.getenv("NODE_PATH")).fold(nodePath)(_ + nodePathDelim + nodePath)
     if (newNodePath.isEmpty) Map.empty[String, String] else Map("NODE_PATH" -> newNodePath)
   }
+
+  // This quoting functionality is as recommended per http://bugs.java.com/view_bug.do?bug_id=6511002
+  // The JDK can't change due to its backward compatibility requirements, but we have no such constraint
+  // here. Args should be able to be expressed consistently by the user of our API no matter whether
+  // execution is on Windows or not.
+  private def needsQuoting(s: String): Boolean =
+    if (s.isEmpty) true else s.exists(c => c == ' ' || c == '\t' || c == '\\' || c == '"')
+
+  private def winQuote(s: String): String = {
+    if (!needsQuoting(s)) {
+      s
+    } else {
+      "\"" + s.replaceAll("([\\\\]*)\"", "$1$1\\\\\"").replaceAll("([\\\\]*)\\z", "$1$1") + "\""
+    }
+  }
+
+  private val isWindows: Boolean = System.getProperty("os.name").toLowerCase.contains("win")
+
+  private[jse] def prepareArgs(args: immutable.Seq[String]): immutable.Seq[String] =
+    if (isWindows) args.map(winQuote) else args
 }
 
 /**

src/main/scala/com/typesafe/sbt/jse/npm/Npm.scala

@@ -3,32 +3,92 @@ package com.typesafe.sbt.jse.npm
 
 import java.io.File
 
-import com.typesafe.sbt.jse.engines.{Engine, JsExecutionResult}
+import com.typesafe.sbt.jse.engines.{Engine, JsExecutionResult, LocalEngine}
 
 import scala.collection.immutable
+import scala.collection.JavaConverters._
 import scala.collection.mutable.ListBuffer
+import scala.sys.process.{Process, ProcessLogger}
+import scala.util.Try
 
 /**
   * A JVM class for performing NPM commands. Requires a JS engine to use.
   */
-class Npm(engine: Engine, npmFile: File, verbose: Boolean = false) {
+class Npm(engine: Engine, npmFile: File, verbose: Boolean = false, preferSystemNpm: Boolean = true) {
 
+  // TODO deprecate all constructors, file not always necessary (when using local installed)
   def this(engine: Engine, npmFile: File) = this(engine, npmFile, false)
 
+  def install(global: Boolean = false, names: Seq[String] = Nil, outSink: String => Unit, errSink: String => Unit): JsExecutionResult = {
+    cmd("install", global, names, outSink, errSink)
+  }
+
   def update(global: Boolean = false, names: Seq[String] = Nil, outSink: String => Unit, errSink: String => Unit): JsExecutionResult = {
+    cmd("update", global, names, outSink, errSink)
+  }
+
+  def ci(names: Seq[String] = Nil, outSink: String => Unit, errSink: String => Unit): JsExecutionResult = {
+    cmd("ci", false, names, outSink, errSink) // ci subcommand does not support -g (global) flag
+  }
+
+  private def cmd(cmd: String, global: Boolean = false, names: Seq[String] = Nil, outSink: String => Unit, errSink: String => Unit): JsExecutionResult = {
     val args = ListBuffer[String]()
-    args += "update"
+    args += cmd
     if (global) args += "-g"
     if (verbose) args += "--verbose"
     args ++= names
     invokeNpm(args, outSink, errSink)
   }
 
+  private def detectNpm(command: String): Option[String] = {
+    val npmExists = Try(Process(s"$command --version").!!).isSuccess
+    if (!npmExists) {
+      println("!!!")
+      println(s"Warning: npm detection failed. Tried the command: $command")
+      println("!!!")
+      None
+    } else {
+      Some(command)
+    }
+  }
+
   private def invokeNpm(args: ListBuffer[String], outSink: String => Unit, errSink: String => Unit): JsExecutionResult = {
     if (!engine.isNode) {
       throw new IllegalStateException("node not found: a Node.js installation is required to run npm.")
     }
-    engine.executeJs(npmFile, args.to[immutable.Seq], Map.empty, outSink, errSink)
+
+    def executeJsNpm(): JsExecutionResult = engine.executeJs(npmFile, args.to[immutable.Seq], Map.empty, outSink, errSink)
+
+    engine match {
+      case localEngine: LocalEngine if preferSystemNpm =>
+        // The first argument always is the command of the js engine, e.g. either just "node", "phantomjs,.. or a path like "/usr/bin/node"
+        // So, if the command is a path, we first try to detect if there is a npm command available in the same folder
+        val localEngineCmd = new File(localEngine.stdArgs.head)
+        val localNpmCmd = if (localEngineCmd.getParent() == null) {
+            // Pretty sure the command was not a path but just something like "node"
+            // Therefore we assume the npm command is on the operating system path, just like the js engine command
+            detectNpm("npm")
+          } else {
+            // Looks like the command was a valid path, so let's see if we can detect a npm command within the same folder
+            // If we can't, try to fallback to a npm command that is on the operating system path
+            val cmdPath = new File(localEngineCmd.getParentFile, "npm").getCanonicalPath
+            detectNpm(cmdPath).orElse(detectNpm("npm"))
+          }
+        localNpmCmd match {
+          case Some(cmd) =>
+            val allArgs = immutable.Seq(cmd) ++ args
+            val pb = new ProcessBuilder(LocalEngine.prepareArgs(allArgs).asJava)
+            pb.environment().putAll(localEngine.stdEnvironment.asJava)
+            JsExecutionResult(Process(pb).!(ProcessLogger(outSink, errSink)))
+          case None =>
+            println("!!!")
+            println(s"Warning: npm detection failed. Falling back to npm provided by WebJars, which is outdated and will not work with Node versions 14 and newer.")
+            println("!!!")
+            executeJsNpm()
+        }
+      case _ => // e.g. Trireme provides node, but is not a local install and does not provide npm, therefore fallback using the webjar npm
+        executeJsNpm()
+    }
   }
 
 }

src/test/scala/com/typesafe/sbt/jse/npm/NpmSpec.scala

@@ -32,7 +32,8 @@ class NpmSpec extends Specification {
       val stdOut = out.toString()
 
       result.exitValue must_== 0
-      stdErr must contain("npm http request GET https://registry.npmjs.org/amdefine")
+      stdErr must contain("npm http request GET https://registry.npmjs.org/amdefine") // when using webjar npm
+        .or(contain("npm http fetch GET 200 https://registry.npmjs.org/amdefine")) // when using local installed npm
     }
   }
 }