fix dependency resolution

[ddtthh]

I've tried the current master branch and found that it fails to resolve js dependencies completely. The dependencies are installed in the respective node_modules directories and listed in package.json, but the way rollup and webpack are run, they fail to find them.

I did not find any command line options of npm/npx nor webpack/rollup to fix this. So my solution is to symlink the contents of npmInstall.dest into bundle.dest and then run webpack/rollup using npx.

I'm not sure this works on windows, but the code uses symlinks already, so I've assumed it is fine, and in theory at least, windows supports symlinks.

Maybe there's a better solution, but as I had to get this running anyway, I wanted to share it.

[nafg]

Maybe I should have reviewed the PR properly

[nafg]

Maybe it could be fixed just by setting the working directory the command is run in?

[nafg]

@ddtthh what would it take to get the tests to catch that? (the issue this PR is to fix)

[ThijsBroersen]

I understand it is not working on your end @ddtthh but the fixes proposed here are running global available rollup and webpack versions. The implementations as-was are using rollup and webpack versions defined in the build and installed in the npmInstall.dest path. Those should be referenced. It was working before 1.x migration and it is still working in master.

Can we perhaps first check you test setup? Like what node versions did you try? Personally I am running 23.11 and on CI the default is 20.19.4.
And perhaps we can then reproduce.

[ThijsBroersen]

Maybe it could be fixed just by setting the working directory the command is run in?

this is already set, e.g.

mill-bundler/millbundler/src/io/github/nafg/millbundler/ScalaJSWebpackModule.scala

Line 103 in ef39f29

cwd = Task.dest

[ddtthh]

I had to investigate a bit why the tests worked, but devBundle failed before my fix, as they use a js dependency as well.

The reason is fastLinkJSTest symlinks node_modules etc. to fastLinkJSTest.dest and then calls bundle.apply(). As this is an anon task it runs in the same destination directory and node will find node_modules etc.

bundleDev and bundleProd also call bundle but without symlinking before, so the dependencies are not available.

My fix is basically moving this to bundle. The functionality could be removed from fastLinkJSTest symlinks node_modules etc. to fastLinkJSTest.dest and then calls bundle.apply(). As this is an anon task it runs in the same destination directory and node will find node_modules etc.

bundleDev and bundleProd also call bundle but without symlinking before, so the dependencies are not available.

My fix is basically moving this to bundle. The functionality could be removed from fastLinkJSTest.

With respect to using npx: that could be reverted back to calling node directly. npx does use the locally installed version as well, I do not have a globally installed webpack/rollup. But npx does not seem to be available in the windows integration pipeline. This seems to be why the tests fail now for windows. We could also call "npm exec webpack/rollup".

I've created a sample project to test this: https://github.com/ddtthh/millbundler-example
mill clean; mill devBundle fails for master before my pull request and it works after.

Some unrelated remarks:

• mill run does not work with js dependencies either, but this just has not been implemented yet
• mill docs say plugins should use the mill version prefix.

[ddtthh]

Maybe it could be fixed just by setting the working directory the command is run in?

this is already set, e.g.

mill-bundler/millbundler/src/io/github/nafg/millbundler/ScalaJSWebpackModule.scala

Line 103 in ef39f29

cwd = Task.dest

It could be set to npmInstall.dest, then the output would somehow have to be explicitly be set to be generated in Task.dest. But I had issues with non-standard project layouts and files outside of the project trees in the past. It's probably most robust to setup a standard project layout with input and output files contained in sub directories of the directory containing node_modules and package.js.

[ThijsBroersen]

@ddtthh
I have tried to test more locally and continued changing some things, including symlinking required files to the Task.dest directories to make less complex.
My goal was also to try and support modules with multiple emitted modules but it looks like only SBT supports this atm. In the Mill source-code I could not find a way to get this done.
In my case I have a module which emits library code as well as multiple worker applications. My solution for Mill is to just create new modules, each building a single module.
#71