Support for custom webpack config in reload workflow #119

jeroentervoorde · 2017-04-05T19:31:21Z

I implemented this when i ran into an issue using leaflet assets which contain images and require loader configuration (according to my very minimal webpack knowledge..). This failed when using reload workflow. The bundled js is now created using the optional webpack configuration set by "webpackConfigFile in webpackReload"

To be able to scope the setting i added the fastOptJS::webpackReload task which does the same thing as fastOptJS::webpack with reloadWorkflow enabled.

The reloadWorkflow setting still works and also supports the webpackConfigFile setting.

If the approach is ok i'll try to add a test.

- Added a task fastOptJS::webpackReload equivalent to fastOptJS::webpack with reloadWorflow := true. - Support webpack config using webpackConfigFile setting scoped to webpackReload task

scala-jenkins · 2017-04-05T19:31:23Z

Can one of the admins verify this patch?

julienrf · 2017-04-05T20:01:35Z

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

-    Webpack.run(entryPoint.absolutePath, bundleFile.absolutePath)(workingDir, logger)
+    customWebpackConfigFile match {
+      case Some(configFile) =>
+        Webpack.run("--config", configFile.getAbsolutePath, entryPoint.absolutePath, bundleFile.absolutePath)(workingDir, logger)


Does that mean that webpack still uses the given entryPoint and bundleFile instead of using those given in the configuration file?

No, it doesn't. I created a stripped down version of my webpack config containing only loader and rules configuration:

module.exports = { module: { loaders: [ { test: /\.(jpe?g|png|gif|svg)$/i, loaders: [ 'file?hash=sha512&digest=hex&name=[hash].[ext]', 'image-webpack?bypassOnDebug&optimizationLevel=7&interlaced=false' ] } ], rules: [ { test: /\.css$/, use: [ 'style-loader', 'css-loader' ] }, { test: /\.(png|jpg|gif|svg|eot|ttf|woff|woff2)$/, loader: 'url-loader', options: { limit: 10000 } } ] } }

I tried if the command option for entry point would override the value in the config file but unfortunately it does not.

That’s great to be able to use a custom configuration file as you do, but I think that’s not good if we have to duplicate parts of the config between the “reload workflow” and the usual workflow.

I think that would be easy to share the same config, though. For instance, you could have a webpack-module.js file containing the code you pasted. Then we could have a webpack.config.js file that would import this one… That’s a bit complex but that would work…

Cool, could you add a test that does that?

That would work i guess. One problem is that the custom webpack file is copied to the target dir which. Therefore the 'module' config needs to be imported using an absolute path. ( I could just copy the module config to the target dir as well ofcourse)

Maybe a better approach would be to actually add a config setting for the module-config only. This can than be used for the reload workflow but also be automatically included in the generated webpack config. A custom config file would still need to manually include the module config but it would be located in the same directory where the custom file is copied.

I imported my custom config using
var modulesConfig = require('/home/.../project/js/webpack-reload.config.js')

modulesConfig.modules should than be merged with the configuration.

Is this the way to do it?

I could just copy the module config to the target dir as well of course

That’s what we do with the custom webpack config file.

Maybe a better approach would be to actually add a config setting for the module-config only.

I’m not sure about that. This configuration entry can contain arbitrary JavaScript expressions (for instance a JavaScript RegExp). Providing support for that would require some effort (unless we make it possible to write the configuration module in Scala.js). That’s why so far I tried to make as few configuration settings as possible available from scalajs-bundler: for other use cases you should fallback to a plain JavaScript config file.

Actually i meant a setting for the module config file that would replace the "webpackConfigFile in reloadWorkflow" i use now. This file can than be copied to target and used in reloadWorkflow and included in the generated webpack config (using require and maybe some js for merging the settings). Because it is copied it can also be included in a custom config using a relative filename.

…tion - Added webpackSharedConfigFiles containing a list of files to be copied to the target dir. - Copy reload config file to target dir so it can include a shared config using a relative path. - Implement custom config for test runner (webpackConfigFile in test) - Added sharedconfig scripted test.

jeroentervoorde · 2017-04-08T14:34:30Z

I added a setting webpackSharedConfigFiles which contains a list of files copied to the webpack working dir. This file can than be included in a webpackCustomConfig file.

A test (sharedconfig) was added as a working example. While writing the test i noticed that the test runner also did not support a custom webpack file so i added this ("webpackCustomConfig in test") which can normally point to the same file as "webpackCustomConfig in webpackReload"

In the example the custom config file and the shared config could have been 1 file. In this case it would have been copied twice but i assume this is not a problem.

The pr needs more scaladoc. I'll add that if we agree on this implementation.

julienrf · 2017-04-08T18:53:16Z

Hi, I think this is great! (and that would also probably solve #83)

However, I would propose the following change. Instead of having this sharedWebpackConfigFiles setting I would simply copy all the .js files that are in the baseDirectory. Actually, this should be configurable, so we would have a webpackResources setting of type PathFinder that would default to baseDirectory.value ** "*.js" (thus, that would include both the config files and the shared config files). Then we would copy all the webpackResources into the target directory before invoking webpack. What do you think? (Also, that might help us to fix #112)

If you could update the PR with that that would be awesome! (Then I might add some minor comments, but I would first like to validate the general solution)

jeroentervoorde · 2017-04-11T07:48:35Z

Sure, i'll update it this week. My plan is to append the webpackConfigFile to the files returned by webpackResources as it may not be location in the project root. For #112 we could than hash over all these config files. Another option would be to enforce that webpackConfigFile is part of webpackResources but that's a bit more limiting.

The idea I had for the sharedWebpackConfigFiles was that it could be used as to implement merging in the generated config files as well (in a new PR). This can still be done but would still require a setting besides webpackResources.

jeroentervoorde · 2017-04-16T08:49:56Z

Hi! Updated again. sharedWebpackConfigFiles -> webpackResources (using baseDirectory.value * "*.js" as default so only the files in the project root are copied)

webpackCustomConfig is copied as well if it wasn't by webpackResources.

julienrf · 2017-04-18T13:01:13Z

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

@@ -6,6 +6,14 @@ import scalajsbundler.util.{Commands, JS}

 object Webpack {

+  def copyToWorkingDir(targetDir: File)(file: File): File = {
+    val copy = targetDir / file.name
+    if (copy.getAbsolutePath != file.getAbsolutePath && !copy.exists()) {


Why are these tests necessary? Does that happen to have the same source and destination? In such a case maybe we should let the IO.copyFile method throw an error. Also, the second test will make it impossible to update a file that has been copied and then changed afterwards. Unless I’m missing something?

Your're right the first test isn't really necessary and the second is plain wrong (i was assuming an empty target dir). I removed them both.

julienrf · 2017-04-18T13:04:35Z

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

+                  val copy = targetDir / file.name
+                  IO.copyFile(file, copy)
+                  copy
+                }


Why can’t you reuse the one defined in the Webpack object?

Missed that one.. Updated now

julienrf · 2017-04-18T13:07:36Z

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

@@ -465,6 +502,10 @@ object ScalaJSBundlerPlugin extends AutoPlugin {
        else webpackTask(fastOptJS)
      }.value,

+      webpackReload in fastOptJS := Def.taskDyn {
+        ReloadWorkflowTasks.webpackTask(configuration.value, fastOptJS)
+      }.value,


Why is this a taskDyn rather than a task?

I tried changing it to a normal task but it doesn't compile. I assume because ReloadWorkflowTasks.webpackTask returns an Def.Initialize[Task[...]]

julienrf · 2017-04-18T13:24:11Z

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

@@ -546,6 +587,24 @@ object ScalaJSBundlerPlugin extends AutoPlugin {
                val sjsOutputName = sjsOutput.name.stripSuffix(".js")
                val bundle = targetDir / s"$sjsOutputName-bundle.js"

+                val customWebpackConfigFile = (webpackConfigFile in test).value


Why not use the Test configuration instead of the test task?

julienrf

That looks really great! Could you also add more documentation about which entries are required (which should not be present) when supplying a config file for tests or reload workflow?

julienrf · 2017-04-18T13:24:45Z

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

+// Use a different Webpack configuration file for reload workflow
+webpackConfigFile in webpackReload := Some(baseDirectory.value / "reload.webpack.config.js")
+
+webpackSharedConfigFiles := Seq(baseDirectory.value / "common.webpack.config.js")


Why not leave the default setting?

dispalt · 2017-04-20T13:44:26Z

So just to understand this, does this allow us to have a webpack conf and still use the fast reload workflow?

julienrf · 2017-04-20T13:44:37Z

test this please

julienrf · 2017-04-20T13:44:44Z

@dispalt yes :)

julienrf · 2017-04-20T14:00:04Z

@jeroentervoorde I pushed a commit to fix the tests. Could you please take into account my other remarks?

scala-jenkins · 2017-04-20T14:01:52Z

Refer to this link for build results (access rights to CI server needed):
https://scala-webapps.epfl.ch/jenkins//job/scalajs-bundler/126/
Test FAILed.

julienrf · 2017-04-20T14:04:31Z

OK, I don’t know why but jenkins ignores my last push…

Updated documentation and sharedconfig test project

julienrf · 2017-04-23T20:23:58Z

test this please

julienrf · 2017-04-23T20:29:46Z

sbt-scalajs-bundler/src/sbt-test/sbt-scalajs-bundler/sharedconfig/common.webpack.config.js

+          'image-webpack?bypassOnDebug&optimizationLevel=7&interlaced=false'
+        ]
+      }
+    ],


According to the CI (line 1170) it seems that at some point a require('fs') is performed from the browser… Do you know why? Do the tests run fine on your machine?

I don't know. I ignored it because everything seems to run fine. I'll try to fix it tonight or tomorrow.

At least the tests passed on the *nix CI. We have so many troubles with Windows support :(

Hi. I fixed it using the additional "node" configuration described here:
pugjs/pug-loader#8

Hmm, but that’s weird because the final target is the web browser, not node, right?

I guess so.. Is the target something i can/should change? I couldn't find it in the code.

julienrf

Excepted the error in the CI that looks awesome, thanks!

scala-jenkins · 2017-04-23T20:43:37Z

Refer to this link for build results (access rights to CI server needed):
https://scala-webapps.epfl.ch/jenkins//job/scalajs-bundler/128/
Test PASSed.

julienrf · 2017-04-24T09:16:00Z

manual/src/ornate/cookbook.md

+Here are the steps to share the loader configuration among your prod and dev config files. This
+uses webpack-merge for convenience the same result could be accomplished using plain js only.
+
+1. Put configuration it in a common.webpack.config.js file:


Typo: extra “it”

Fixed documentation typo's

julienrf · 2017-04-26T11:57:48Z

Thank you so much!

Support custom webpack config in reload workflow

49e14f9

- Added a task fastOptJS::webpackReload equivalent to fastOptJS::webpack with reloadWorflow := true. - Support webpack config using webpackConfigFile setting scoped to webpackReload task

julienrf reviewed Apr 5, 2017

View reviewed changes

Replaced sharedWebpackConfigFiles by webpackResources.

c8a67e0

julienrf reviewed Apr 18, 2017

View reviewed changes

fix tests

11e74b5

Processed review comments

155f49a

Updated documentation and sharedconfig test project

julienrf reviewed Apr 23, 2017

View reviewed changes

julienrf approved these changes Apr 23, 2017

View reviewed changes

julienrf reviewed Apr 24, 2017

View reviewed changes

Fixed 'fs' module error on CI

73ea4cd

Fixed documentation typo's

julienrf merged commit a3d585b into scalacenter:master Apr 26, 2017

Support for custom webpack config in reload workflow #119

Support for custom webpack config in reload workflow #119

Conversation

jeroentervoorde commented Apr 5, 2017

scala-jenkins commented Apr 5, 2017

Choose a reason for hiding this comment

jeroentervoorde Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeroentervoorde Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeroentervoorde commented Apr 8, 2017

julienrf commented Apr 8, 2017

jeroentervoorde commented Apr 11, 2017

jeroentervoorde commented Apr 16, 2017

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

julienrf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dispalt commented Apr 20, 2017

julienrf commented Apr 20, 2017

julienrf commented Apr 20, 2017

julienrf commented Apr 20, 2017

scala-jenkins commented Apr 20, 2017

julienrf commented Apr 20, 2017

julienrf commented Apr 23, 2017

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

julienrf left a comment

Choose a reason for hiding this comment

scala-jenkins commented Apr 23, 2017

Choose a reason for hiding this comment

julienrf commented Apr 26, 2017

jeroentervoorde Apr 6, 2017 •

edited

Loading

jeroentervoorde Apr 6, 2017 •

edited

Loading