Failure to parse Webpack stats fails the build #424

ptrdom · 2022-05-13T18:36:19Z

Failure to parse Webpack stats can fail builds with certain Webpack configs. It should probably be a critical failure, rather than logging with return of an Option.

ptrdom · 2022-05-13T18:36:39Z

Related to #423.

evbo · 2022-05-14T20:03:03Z

and related to #338 which was closed because I presumed the PR would get merged, but it never did - shame on me :)

evbo · 2022-05-14T23:41:20Z

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

@@ -115,13 +115,13 @@ object Stats {
  )(Asset.apply _)

  implicit val errorReads: Reads[WebpackError] = (


@ptrdom what do you think of just using semi-auto derivation in Stats.scala?:

implicit val assetsReads: Reads[Asset] = Json.reads[Asset] implicit val errorReads: Reads[WebpackError] = Json.reads[WebpackError] implicit val warningReads: Reads[WebpackWarning] = Json.reads[WebpackWarning] implicit val pathReads: Reads[Path] = Reads.StringReads.map(new File(_).toPath) implicit val statsReads: Reads[WebpackStats] = Json.using[Json.WithDefaultValues].reads[WebpackStats]

I saw this used in #408 and tested this works and saves a few lines of code ;)

evbo · 2022-05-14T23:41:37Z

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

@@ -50,9 +50,9 @@ object Stats {

  }

-  final case class WebpackError(moduleName: String, message: String, loc: String)
+  final case class WebpackError(moduleName: Option[String], message: String, loc: String)


the docs are a little unclear, but besides message it sounds like "various" other properties means perhaps loc should be optional too?:
https://webpack.js.org/api/stats/#errors-and-warnings

evbo · 2022-05-14T23:45:34Z

Also, @ptrdom did you try sbt publishLocal with your fork? I tried it and my project failed to resolve it as a dependency due to scalajs-bundler-linker not getting published locally. I had to add it to the build.sbt aggregate statement here:
https://github.com/scalacenter/scalajs-bundler/blob/main/build.sbt#L90

Curious to know how you managed to publish locally without that fix...

ptrdom · 2022-05-15T05:58:02Z

@evbo I ran into the same issue and simply published the module explicitly, same as the CI workflow does it - https://github.com/scalacenter/scalajs-bundler/blob/main/.github/workflows/release.yml#L21-L22. Adding the project to aggregate makes sense to me.

ptrdom · 2022-05-15T07:17:03Z

@sjrd Can we merge this?

sjrd · 2022-05-16T08:42:01Z

build.sbt

-    .aggregate(`sbt-scalajs-bundler`, `sbt-web-scalajs-bundler`)
+    .aggregate(`sbt-scalajs-bundler`, `sbt-web-scalajs-bundler`, `scalajs-bundler-linker`)


This shouldn't be in this PR.

Moreover, this is wrong anyway, because scalajs-bundler-linker doesn't have the same cross-version setup as the other projects.

that was my bad - whoops. See comment: #424 (comment)

Note: I am currently using scalajs-bundler published locally with that aggregate and it has been running fine, though I'm not sure of the implications to a different "cross-version setup".

While I do understand that this change does not have to be with this PR, but there is still an issue related to publishing in development setup, rather than cross-version setup.

Current documentation for contributing notes that sbt publishLocal is enough for local publishing, but that is false, since scalajs-bundler-linker will not be published and it will result in errors. So either documentation needs to be updated or scalajs-bundler-linker project has to be added to the aggregate.

Publishing workflow works around that by publishing each module individually -
https://github.com/scalacenter/scalajs-bundler/blob/main/.github/workflows/release.yml#L21-L22

@sjrd Am I correct that there is an issue and a solution is required?

sjrd · 2022-05-16T09:03:30Z

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

+  implicit val errorReads: Reads[WebpackError] = Json.reads[WebpackError]
+  implicit val warningReads: Reads[WebpackWarning] = Json.reads[WebpackWarning]
+  implicit val pathReads: Reads[Path] = Reads.StringReads.map(new File(_).toPath)
+  implicit val statsReads: Reads[WebpackStats] = Json.using[Json.WithDefaultValues].reads[WebpackStats]


These changes don't seem to have anything to do with the purpose of this PR, have they?

@sjrd It does because you need to update the JSON decoders to parse the new Option types that have been added. So rather than explicitly modify the individual fields it parses as Options, this solves the problem using derived decoders, which is also fewer lines and I don't think there are any downsides since it's compile-time derivation (maybe slightly longer time taken compiling?)

Since the model has changed, Reads do have to be modified, and derived decoders are a simplification of those changes.

sjrd · 2022-05-16T09:11:34Z

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

+      if (notExisting.nonEmpty) {
+        throw new RuntimeException(
+          s"Webpack failed to create application assets:\n" +
+            s"${notExisting.map(asset => s"${asset.getAbsolutePath}").mkString("\n")}")


I you're going to throw anyway, this method can be simplified a lot:

val assets = stats.resolveAllAssets(targetDir) val nonExisting = assets.filter(!_.exists()) if (nonExisting.nonEmpty) { throw ... } assets

sjrd · 2022-05-16T09:14:23Z

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

+      .fold(
+        sys.error,


If you're going to throw when the Try is a Failure, you as well do

val runResult = Commands.run(...).get

But then again, why change Commands.run to return a Try in that case, instead of directly throwing the exception?

I tried to modify the existing code as little as possible, I see that expression was lost in a way with nesting of all these monads.

Command.run returns an Either[String, Try[T]]] where:

Left means that command returned with a non-zero exit code, so we sys.error it.

Right(None) means that command returned with a zero exit code, but had no output, so we throw a throw new RuntimeException("Failure on returning webpack stats from command output")

Right(Some(Failure)) means that command returned with zero exit code and had output, but output parsing to WebpackStats with jsonOutput has failed.

Right(Some(Success) means that command returned with zero exit code, had output and output parsing to WebpackStats with jsonOutput has succeeded.

So current implementation looks more funky because I wanted to separate the cases 2) and 3) - failures are related to command output, but mean different things.

Maybe this would have looked clearer?

Commands.run(cmd, workingDir, log, jsonOutput(cmd, log)) .fold( sys.error, _.fold(throw new RuntimeException("Command succeeded, but returned no webpack stats output"))(_.get) )

Otherwise Command.run would benefit from an improvement to return type specificity.

Knowing this, what further modifications would you suggest?

sjrd · 2022-05-16T09:15:47Z

sbt-scalajs-bundler/src/main/scala/scalajsbundler/util/Commands.scala

 import sbt.Logger
-import java.io.{InputStream, File}
+import java.io.{File, InputStream}
+
 import scala.sys.process.Process


Spurious changes.

sjrd · 2022-05-16T09:17:24Z

sbt-scalajs-bundler/src/sbt-test/sbt-scalajs-bundler/webpack-stats/test

@@ -0,0 +1 @@
+> fullOptJS::webpack


I don't understand how this test relates to the fix. If this PR is about failing a build, there should a be at least one negative test line (starting with -).

This test would fail in current main branch, the changes added introduce fixes that actually make it pass. Is that fine?

ptrdom · 2022-05-20T17:45:44Z

@sjrd I made changes for which I did not have further questions, please review the discussions and the code when you can.

annedroiid · 2022-11-14T11:45:05Z

@sjrd Are the latest changes acceptable? Do you need any help with this?

rmmeans · 2024-02-13T12:27:56Z

Wondering if this PR will be up for consideration in the future? I know many are moving to Vite, but our current build process is causing problems with Vite since we're packaging node modules for browser (Autoprefixer and postcss). Didn't have much luck working through (tried following advice postcss/postcss#830, but wasn't able to get it to work).

Was hoping to upgrade to webpack5 on scalajs-bundler until we can refactor how we do CSS to not need autoprefixer running in the browser and then switch to Vite.

ptrdom marked this pull request as draft May 13, 2022 18:37

ptrdom marked this pull request as ready for review May 14, 2022 07:43

ptrdom force-pushed the webpack-stats-parse-failure branch from 8170f89 to bee2882 Compare May 14, 2022 17:30

evbo reviewed May 14, 2022

View reviewed changes

sjrd requested changes May 16, 2022

View reviewed changes

ptrdom added 6 commits May 20, 2022 20:43

Minimal test case for the issue

f5aa4a8

Refactor webpack stats error handling to fail build on error

a42a8a3

Refactor webpack stats module names in warning and error to optional

3912d62

Minor improvements

d83aea8

Minor improvements

ecb5b6f

Various fixes

d33cb21

ptrdom force-pushed the webpack-stats-parse-failure branch from d167f25 to d33cb21 Compare May 20, 2022 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Failure to parse Webpack stats fails the build #424

Failure to parse Webpack stats fails the build #424

ptrdom commented May 13, 2022

ptrdom commented May 13, 2022

evbo commented May 14, 2022

evbo May 14, 2022

evbo May 14, 2022

evbo commented May 14, 2022

ptrdom commented May 15, 2022

ptrdom commented May 15, 2022

sjrd May 16, 2022

evbo May 16, 2022

ptrdom May 16, 2022

sjrd May 16, 2022

evbo May 16, 2022

ptrdom May 16, 2022

sjrd May 16, 2022

sjrd May 16, 2022

ptrdom May 16, 2022

sjrd May 16, 2022

sjrd May 16, 2022

ptrdom May 16, 2022

ptrdom commented May 20, 2022

annedroiid commented Nov 14, 2022

rmmeans commented Feb 13, 2024

		@@ -115,13 +115,13 @@ object Stats {
		)(Asset.apply _)

		implicit val errorReads: Reads[WebpackError] = (

		.aggregate(`sbt-scalajs-bundler`, `sbt-web-scalajs-bundler`)
		.aggregate(`sbt-scalajs-bundler`, `sbt-web-scalajs-bundler`, `scalajs-bundler-linker`)

Failure to parse Webpack stats fails the build #424

Are you sure you want to change the base?

Failure to parse Webpack stats fails the build #424

Conversation

ptrdom commented May 13, 2022

ptrdom commented May 13, 2022

evbo commented May 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evbo commented May 14, 2022

ptrdom commented May 15, 2022

ptrdom commented May 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptrdom commented May 20, 2022

annedroiid commented Nov 14, 2022

rmmeans commented Feb 13, 2024