Package manager abstraction #420

ptrdom · 2022-05-05T17:31:38Z

References #417.

Main idea is to make a package manager abstraction that allows a flexible interaction with plugin's internals so that user's can extend the behavior however they wish.

TODO:

Implement pnpm.
Fix scripted tests.

ptrdom · 2022-05-05T17:37:05Z

@julienrf @sjrd comments would be appreciated. Could I get some permissions to run CI at will, scripted tests are kind of clunky on my local machine. I could fix up tests that were broken prior to this PR too, but I will need to edit actual CI scripts because I have a feeling it needs some tinkering with Node/npm versions. I do not see where those AppVeyor scripts are located, but do we even need them with GitHub Actions being already in place?

ptrdom · 2022-05-05T17:41:00Z

Also, this very much breaks the backwards compatibility, but I assume that it is fine since the plugin is not v1 yet 😅 Let me know if that is not alright, there is definitely a way to stay backwards compatible, although the old design should be deprecated eventually - things like having both npm and yarn settings available simultaneously are obviously clunky.

ptrdom · 2022-05-09T14:07:56Z

@julienrf @sjrd Could you review this?

sjrd · 2022-05-09T14:13:13Z

I will try to find some time for it this week.

sjrd · 2022-05-16T11:38:10Z

sbt-scalajs-bundler/src/main/scala/scalajsbundler/sbtplugin/ScalaJSBundlerPlugin.scala

+        packageManager.value match {
+          case aps: AddPackagesSupport =>
+            aps.addPackages(baseDir, installDir, log)(s"jsdom@$jsdomVersion")
+        }


There should be a case _ => here, which shows a clear error message to the user, through a throw new MessageOnlyException("error message here").

sjrd · 2022-05-16T11:38:54Z

sbt-scalajs-bundler/src/main/scala/scalajsbundler/sbtplugin/ScalaJSBundlerPlugin.scala

-    val useYarn: SettingKey[Boolean] =
-      settingKey[Boolean]("Whether to use yarn for updates")


We should preserve all these keeps as @deprecated for compatibility reasons.
We can initialize packageManager based on the value of those deprecated settings.

sjrd · 2022-05-16T11:39:18Z

sbt-scalajs-bundler/src/main/scala/scalajsbundler/sbtplugin/ScalaJSBundlerPlugin.scala

-      */
-    val useYarn: SettingKey[Boolean] =
-      settingKey[Boolean]("Whether to use yarn for updates")
+    val packageManager = SettingKey[PackageManager](


This should be called jsPackageManager, to reduce the likelihood of collision with other sbt plugins.

sjrd · 2022-05-16T11:41:41Z

sbt-scalajs-bundler/src/sbt-test/sbt-scalajs-bundler/custom-test-config/build.sbt

-useYarn := true
+packageManager := scalajsbundler.Yarn()


We should therefore keep a test that uses the deprecated useYarn, in addition to the test that uses the new packageManager.

sjrd · 2022-05-16T11:41:54Z

sbt-scalajs-bundler/src/sbt-test/sbt-scalajs-bundler/additonalNpmConfig/build.sbt

-      npmExtraArgs in Compile := Seq("-silent"),
+      packageManager in Compile := Npm(installArgs = Seq("-silent"), addPackagesArgs = Seq("-silent")),


Same here. We'll need to keep one test that uses npmExtraArgs.

sjrd · 2022-05-16T12:06:21Z

sbt-scalajs-bundler/src/main/scala/scalajsbundler/sbtplugin/NpmUpdateTasks.scala

@@ -21,12 +22,10 @@ object NpmUpdateTasks {
  def npmUpdate(baseDir: File,


We should keep a deprecated overload of this method for compatibility reasons, which will delegate to this one.

Also the Scaladoc needs updating.

sjrd · 2022-05-16T12:06:26Z

sbt-scalajs-bundler/src/main/scala/scalajsbundler/sbtplugin/NpmUpdateTasks.scala

@@ -45,18 +44,16 @@ object NpmUpdateTasks {
  def npmInstallDependencies(baseDir: File,


sjrd · 2022-05-16T12:07:21Z

sbt-scalajs-bundler/src/main/scala/scalajsbundler/sbtplugin/PackageJsonTasks.scala

@@ -31,7 +31,8 @@ object PackageJsonTasks {
    webpackVersion: String,
    webpackDevServerVersion: String,
    webpackCliVersion: String,
-    streams: Keys.TaskStreams
+    streams: Keys.TaskStreams,
+    packageManager: PackageManager
  ): BundlerFile.PackageJson = {


Same here, and for the other methods below. We should keep deprecated aliases.

sjrd · 2022-05-16T12:09:32Z

sbt-scalajs-bundler/src/sbt-test/sbt-scalajs-bundler/npm/test

@@ -0,0 +1,2 @@
+> fastOptJS::webpack
+$ exists package-lock.json


Missing EOL.

sjrd · 2022-05-16T12:09:43Z

sbt-scalajs-bundler/src/sbt-test/sbt-scalajs-bundler/yarn-interactive/test

@@ -1 +1,2 @@
 > fastOptJS::webpack
+$ exists yarn.lock


Missing EOL.

ptrdom · 2022-05-16T15:22:12Z

@sjrd Thank you for a very thorough review! I will make the changes as requested.

ptrdom · 2022-05-21T09:37:18Z

.github/workflows/ci.yml

+      - name: Enable Corepack
+        run: corepack enable
      - name: Setup yarn
-        run: npm install -g yarn@1.22.15
+        run: corepack prepare yarn@1.22.15 --activate
+      - name: Setup pnpm
+        run: corepack prepare pnpm@7.0.1 --activate


Managing package manager installations inside and outside Corepack do not mix well, so we got to do it with Corepack only.

ptrdom · 2022-05-21T10:26:48Z

@sjrd I think I made all of the modifications you requested, PR is ready for a review. I also updated docs.

ptrdom · 2022-06-05T13:22:36Z

sbt-scalajs-bundler/src/main/scala/scalajsbundler/PackageManager.scala

+  }
+
+  trait LockFileSupport {
+    val lockFileName: String


It would be better to convert this to a File to allow for better lockfile location customization.

Though I do not have the best idea on where to put PackageManager.Npm and PackageManager.Yarn so that baseDirectory is available during construction.

Currently scalajs-bundler will always update the lock file, making working directory dirty on CI. See scalacenter/scalajs-bundler#420 and scalacenter/scalajs-bundler#406

mims-github · 2023-02-22T09:23:29Z

@ptrdom Thanks for this PR, would love to use pnpm with Scala.js. :)

Anything missing for this PR to be merged?

threeseed · 2023-06-29T11:12:50Z

@sjrd .. Can we request a review of this. I would be happy to add bun support as well on top of this.

Either way the JS community has much faster options than NPM right now which we should take advantage of.

ptrdom · 2023-06-29T12:46:34Z

I think this PR was ignored for long enough to assume that maintainers are not accepting any PRs to actually develop features, which is fine, but it would have been considerate towards community to explicitly state that.

My suggestions for plugin users who want a package manager abstraction:

Fork scalajs-bundler and publish a community fork.
Move on to Vite in officially promoted way:
- https://www.scala-js.org/doc/tutorial/scalajs-vite.html
Move on to one of the plugins I worked on:
- https://github.com/ptrdom/scalajs-vite for Vite, feature complete, but I am not working on it any further since I personally think that esbuild is a better bundler for Scala.js.
- https://github.com/ptrdom/scalajs-esbuild for esbuild, feature complete for web, still working on Electron plugin, and lacks docs, so I would still call it WIP.

ptrdom changed the title ~~Package managers~~ Package manager abstraction May 5, 2022

ptrdom force-pushed the package-managers branch from 843f342 to 21ee699 Compare May 8, 2022 13:35

This was referenced May 9, 2022

is npm ci supported? #406

Closed

Add first-class support for frozen lockfiles #409

Open

ptrdom force-pushed the package-managers branch from 7c1b1b6 to 0b635f7 Compare May 15, 2022 16:58

sjrd requested changes May 16, 2022

View reviewed changes

ptrdom force-pushed the package-managers branch 7 times, most recently from 4ef34f7 to e7413d9 Compare May 21, 2022 08:48

Implement package manager abstraction

2d414ab

ptrdom force-pushed the package-managers branch from e7413d9 to 2d414ab Compare May 21, 2022 09:30

ptrdom commented May 21, 2022

View reviewed changes

sjrd mentioned this pull request Jun 2, 2022

Providing nodeExecCommand setting #427

Open

ptrdom commented Jun 5, 2022

View reviewed changes

ptrdom mentioned this pull request Jun 5, 2022

Yarn 2 throws Unsupported option name ("--mutex") #428

Open

julienrf requested a review from sjrd February 27, 2023 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Package manager abstraction #420

Package manager abstraction #420

ptrdom commented May 5, 2022 •

edited

Loading

ptrdom commented May 5, 2022

ptrdom commented May 5, 2022 •

edited

Loading

ptrdom commented May 9, 2022

sjrd commented May 9, 2022

sjrd May 16, 2022

sjrd May 16, 2022

sjrd May 16, 2022

sjrd May 16, 2022

sjrd May 16, 2022

sjrd May 16, 2022

sjrd May 16, 2022

sjrd May 16, 2022

sjrd May 16, 2022

sjrd May 16, 2022

ptrdom commented May 16, 2022

ptrdom May 21, 2022

ptrdom commented May 21, 2022

ptrdom Jun 5, 2022

ptrdom Jun 5, 2022

mims-github commented Feb 22, 2023

threeseed commented Jun 29, 2023

ptrdom commented Jun 29, 2023

		val useYarn: SettingKey[Boolean] =
		settingKey[Boolean]("Whether to use yarn for updates")

		npmExtraArgs in Compile := Seq("-silent"),
		packageManager in Compile := Npm(installArgs = Seq("-silent"), addPackagesArgs = Seq("-silent")),

		@@ -21,12 +22,10 @@ object NpmUpdateTasks {
		def npmUpdate(baseDir: File,

		@@ -45,18 +44,16 @@ object NpmUpdateTasks {
		def npmInstallDependencies(baseDir: File,

		@@ -0,0 +1,2 @@
		> fastOptJS::webpack
		$ exists package-lock.json

Package manager abstraction #420

Are you sure you want to change the base?

Package manager abstraction #420

Conversation

ptrdom commented May 5, 2022 • edited Loading

ptrdom commented May 5, 2022

ptrdom commented May 5, 2022 • edited Loading

ptrdom commented May 9, 2022

sjrd commented May 9, 2022

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

ptrdom commented May 16, 2022

Choose a reason for hiding this comment

ptrdom commented May 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mims-github commented Feb 22, 2023

threeseed commented Jun 29, 2023

ptrdom commented Jun 29, 2023

ptrdom commented May 5, 2022 •

edited

Loading

ptrdom commented May 5, 2022 •

edited

Loading