build: migration to Mill 1 - #67
Conversation
|
linux build passed! windows build failed because you deleted mill.bat Re VCS version, could you just recreate the behavior by shelling out to |
|
Actually VcsVersion is built into Mill now |
|
added mill.bat again |
|
@nafg I restored the VcsVersion mechanism! |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the millbundler project from Mill 0.12.14 to Mill 1.0.4-native, updating the build system, API usage patterns, and dependency management approach.
- Migration from Mill 0.12.x API to Mill 1.x API with updated import patterns and task definitions
- Change from
ivyDepstomvnDepsfor dependency management following Mill 1.x conventions - Update from Scala 2.13.16 to Scala 3.7.2 and modernize version dependencies
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| build.mill | Updates build configuration to use Mill 1.x APIs and mvnDeps instead of ivyDeps |
| .mill-version | Updates Mill version from 0.12.14 to 1.0.4-native |
| mill, mill.bat | Updates Mill launcher scripts to support Mill 1.x features |
| test_common/src/io/github/nafg/millbundler/testcommon/BaseSuite.scala | Migrates test infrastructure to Mill 1.x APIs with updated imports and task definitions |
| BundleParams.scala | Changes inputFile parameter to inputFiles list with backward compatibility |
| ScalaJSBundleModule.scala | Updates bundle processing to handle multiple input files and Mill 1.x Task API |
| ScalaJSWebpackModule.scala | Migrates webpack integration to Mill 1.x with updated task definitions and file handling |
| ScalaJSRollupModule.scala | Updates rollup integration for Mill 1.x compatibility |
| jsdeps modules | Migrates JavaScript dependency management to Mill 1.x APIs |
| test suites | Adds required millDiscover properties for Mill 1.x module discovery |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| bundle.apply()( | ||
| BundleParams( | ||
| report.dest.path / report.publicModules.head.jsFileName, | ||
| report.publicModules.map(report.dest.path / _.jsFileName).toSeq, |
There was a problem hiding this comment.
The .toSeq call is redundant since map on a collection already returns a sequence. This can be simplified to just report.publicModules.map(report.dest.path / _.jsFileName).
| report.publicModules.map(report.dest.path / _.jsFileName).toSeq, | |
| report.publicModules.map(report.dest.path / _.jsFileName), |
There was a problem hiding this comment.
Sorry copilot, but an Iterable is not a Seq. Without the .toSeq it does not compile
| Seq( | ||
| "node", | ||
| rollupPath.toString, | ||
| copied.head.path.toString |
There was a problem hiding this comment.
Using .head on a collection without checking if it's non-empty can cause a runtime exception if the collection is empty. Consider using .headOption with proper handling or ensuring the collection is non-empty before this call.
| copied.head.path.toString | |
| copied.headOption | |
| .map(_.path.toString) | |
| .getOrElse(throw new RuntimeException("No input files were copied; cannot run rollup")) |
There was a problem hiding this comment.
This makes the failure more explicit, good change.
| "devtool" -> "source-map", | ||
| "entry" -> (dir / params.inputFile.last).toString, | ||
| "output" -> ujson.Obj.from(outputCfg.view.mapValues(ujson.Str)), | ||
| "entry" -> (dir / params.inputFiles.head.last).toString, |
There was a problem hiding this comment.
Using .head on a collection without checking if it's non-empty can cause a runtime exception if the collection is empty. Consider using .headOption with proper handling or ensuring the collection is non-empty before this call.
| "entry" -> (dir / params.inputFiles.head.last).toString, | |
| "entry" -> params.inputFiles.headOption | |
| .map(f => (dir / f.last).toString) | |
| .getOrElse(throw new IllegalArgumentException("inputFiles is empty")), |
|
@ThijsBroersen perhaps I should have reviewed all the code before merging it. See #69 but also I belatedly had @copilot review it, if you could take a look at the comments it added |
I have a working migration to Mill 1 in this branch.
VCS version is not available (yet) for Mill 1, so I left it out.
Implementations are changed a bit because Mill 1 is more strict on hermetic behaviour.
I also change the BundleParams.inputFiles to a list, as report.publicModules is plural. It is also a bit strange to me why there was just assumed the head is the main.js and the rest is ignored.
Feel free to revert or change anything on this PR.