-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Webpack 5 and drop previous versions #390
Add support for Webpack 5 and drop previous versions #390
Conversation
The PR now contains changes to parse warning and errors (I removed the |
@@ -1,12 +1,12 @@ | |||
# the server should properly start | |||
> fastOptJS::webpack | |||
# 3 assets and 2 for loader and entry | |||
> html index.html 5 | |||
# 1 assets and 3 for library, loader and app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stats differentiate between files and auxiliaryFiles now
Source)
i.e. map files are now included in returned assets anymore (breaking change
entry: { | ||
app: [path.resolve(resourcesDir, "./entry.js")] | ||
}, | ||
module: { | ||
rules: [ | ||
{ | ||
test: /\.css$/, | ||
use: ["style-loader", MiniCssExtractPlugin.loader, "css-loader"] | ||
use: [MiniCssExtractPlugin.loader, "css-loader"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style-loader
should not be included as html-webpack-plugin
will automatically handle the CSS files
@@ -51,10 +47,14 @@ InputKey[Unit]("html") := { | |||
assert(files.length == assetsCount) | |||
// Check all files are present | |||
assert(files.map(_.data.exists).forall(_ == true)) | |||
// There is only one app file | |||
assert(files.count(_.metadata.get(BundlerFileTypeAttr) == Some(BundlerFileType.Application)) == 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure to understand how this test worked before, as from my understanding LibraryOnly
mode produces three files: library, loader & app.
}) | ||
] | ||
}); | ||
module.exports = merge(generatedConfig, commonConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uglify is not needed anymore (code is automatically minified in production mode, and this plugin is not compatible with Webpack 5)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need terser
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://webpack.js.org/plugins/terser-webpack-plugin/:
if you are using webpack v5 or above you do not need to install this plugin. Webpack v5 comes with the latest terser-webpack-plugin out of the box
1281e3a
to
8465163
Compare
Thanks a lot for opening this PR @vhiairrassary! Is anyone watching this repository interested in helping? |
I could review it as I did webpack 4 but I,m a bit stretched on time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, sorry it took so long to review
It maybe nice to release this as a RC to let other people test it (i could test it on my projects) before a public release
as it contains breaking changes
@@ -287,7 +287,7 @@ webpackDevServerExtraArgs := Seq("--inline") | |||
`webpack` is then called with the following arguments: | |||
|
|||
~~~ | |||
--bail --config <configfile> | |||
--config <configfile> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that we don't want to bail on errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind I read what is no the description
@@ -91,17 +110,28 @@ object Stats { | |||
implicit val assetsReads: Reads[Asset] = ( | |||
(JsPath \ "name").read[String] and | |||
(JsPath \ "size").read[Long] and | |||
(JsPath \ "emitted").readNullable[Boolean] and | |||
(JsPath \\ "chunkNames").read[List[String]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this is a breaking change in the stats
output? If so it would be hard to simultaneously support webpack 4 and 5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is. To reduce the complexity this PR only supports webpack 5 format (it was discussed as part of #350)
* @param extraArgs - additional arguments for webpack-dev-server. | ||
* @param logger - a logger to use for output | ||
*/ | ||
def start( | ||
workDir: File, | ||
configPath: File, | ||
port: Int, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it expected to configure the port via webpack config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes as the invocation method the the dev-server is different with webpack 5 (I mentioned it as a possible breaking change in the issue description)
|
||
version in startWebpackDevServer := "3.4.1", | ||
version in startWebpackDevServer := "3.11.1", | ||
|
||
version in installJsdom := "9.9.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may as well update jsdom
}) | ||
] | ||
}); | ||
module.exports = merge(generatedConfig, commonConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need terser
?
// There is only one library file | ||
assert(files.count(_.metadata.get(BundlerFileTypeAttr) == Some(BundlerFileType.Loader)) == 1) | ||
// And 1 asset (HTML file) | ||
assert(files.count(_.metadata.get(BundlerFileTypeAttr) == Some(BundlerFileType.Asset)) == 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is odd, I'd expect to see the css as an asset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was near 2 months ago, but if I remember correctly the stats for these files has been moved to stats.relatedAssets
(https://webpack.js.org/blog/2020-10-10-webpack-5-release/)
@vhiairrassary Do you think this needs more work? There are a few checkboxes on the description as "pending" |
6ac53a7
to
bfc2e15
Compare
@cquiroz Thank for the review. I just rebased the PR and I think it is good enough for now. If maintainers agree to merge (it means we drop support for webpack < 5, on master at least) we can:
Toughts? On my side I don't have the permission to publish, can you do it please? |
I think we should do an RC for testers, it would be great to have a sort of migration guide but I don’t know how feasible is that considering how different webpack configurations are I can produce releases but not update the documentation site |
bfc2e15
to
a13b9be
Compare
I rebased the PR to verify tests are still ✅ with GitHub Actions. Can I ask you to produce a release for this PR, without merging it, so interested users can test it? It would allow to get feedbacks, find bugs, help us to write a migration guide, etc. Also, if we merge it it will require a bump of the major version due to the breaking changes. It might be a good idea to tackle the migration from |
I'm not sure I can make a release only for test, I could do |
a13b9be
to
5f8ee62
Compare
@@ -52,7 +52,6 @@ TaskKey[Unit]("checkArchive") := { | |||
"assets/webpack-assets-opt-bundle.js", | |||
"assets/webpack-assets-opt-loader.js", | |||
"assets/webpack-assets-opt-library.js", | |||
"assets/webpack-assets-opt-library.js.map", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.map
files are now in related assets, which are not extracted (yet)
It seems this is ready for testing |
Since it was released in v0.21.0 already, could we have ChangeLog page updated with the notes, please? |
webpack-dev-server
invocation changed as well as the output format for errors and warnings)345,12 Kib
instead of345127
)Built at
log line from stats, as in Webpack 5 (sbt is already outputting a similar line after each task).webpack-dev-server
invocation changed, the server port is not part of the command line but it is injected in the generated webpack configuration (might be a breaking for some users).--bail
flag when invokingwebpack
as it does not return a JSON but print the error instderr
directly (parsing is failing because of that)net.sourceforge.htmlunit
in test to support JS from Webpack 5. Bump some npm dependencies in test to support Webpack 5.Currently I am able to use this version to compile a small application using webpack 5 (including
webpack-dev-server
).Remaining tasks for future 🚧:
Links
Raw Webpack outputs
Webpack 5 (development)
Webpack 5 (production)
Webpack 4 (development)
Webpack 4 (production)
Raw warnings
Webpack 5
Webpack 5