Fixing extra and invalid paths in Webpack 5

[weaverryan]

This PR contains:

• [X] bugfix
• [ ] feature
• [ ] refactor
• [ ] documentation
• [ ] other

Are tests included?

• [X] yes
• [] no

Breaking Changes?

• [ ] yes (breaking changes will not be merged unless absolutely necessary)
• [X] no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers: Fixes #238 and fixes #237 (also fixes https://github.com/symfony/webpack-encore/issues/907)

Description

Hi!

I maintain @symfony/webpack-encore, and we recently upgraded to Webpack 5 and have hit issues with the manifest.json file. So, I dug into it :).

This is complex, and I hope someone may have some ideas on how to make this nicer. Here is the situation:

A) As far as I (and the test suite) can tell, chunk.auxiliaryFiles is only needed to get sourcemap files. All other files are available in other ways. Since all chunk.auxiliaryFiles were used before, it caused #237 because when we loop over these files, we re-use the same chunk.name over and over again (and then just add the correct extension like .svg). So, we need to use some of these, but not all of them. To do that, we collect all the "auxiliary files", then check which have not been added to the manifest at the end. Those that are missing (sourcemaps) are added.

B) The second issue - and the one that fixes #238 - is more complex. Basically, if you're using the new "asset modules" system in Webpack 5, then the emitFile() method - which we rely on in the normalModuleLoaderHook is never called. In fact, as far as I can tell, this method was only ever called by the file-loader - https://github.com/webpack-contrib/file-loader/blob/467e6b782bb8bb9fcb54dcb981b5df1a2aeae818/src/index.js#L81 (this is the loaderContext) - it's never called by Webpack directly.

When you use "asset modules", there is no file-loader and so this is never called. According to the Webpack people - https://github.com/webpack/webpack/issues/12637#issuecomment-776185604 - we should rely on asset.info.sourceFilename as the "source of truth". We were already using that, but sort of "incorrectly" - the asset.info.sourceFilename will be equal to something like '../../assets/logo.png' or 'node_modules/something/fonts/lato-normal.abc123.woff2'. So, what we need is the basename of this prefixed by the dirname of asset.name (which will be something like fonts/lato-normal.abc123.woff2).

To test in your project

If you're having this issue, I would love if this fix could be validated:

yarn remove webpack-manifest-plugin
yarn add git://github.com/weaverryan/webpack-manifest-plugin.git#fix-invalid-paths-webpack5

Thanks!

[shellscape]

I would absolutely love to accept this PR, but without tests to guard against regressions I can't. (I do see that you state that tests are TODO).

I believe chunk.auxiliaryFiles should have never been included when getting the file list

The use of auxillaryFiles was introduced here https://github.com/shellscape/webpack-manifest-plugin/commit/2702efb04a0c54da7ce17a082c5022e6ad9e71e6 to help with v5 compat at that time. The moving goalposts of v5 in the early minor versions made it hard to keep up with, as per usual with new webpack majors. It's cool if that's no longer needed, but we'll need to guard against regressions there.

[codecov-io]

Codecov Report

Merging #249 (6590879) into master (9f408f6) will decrease coverage by 8.96%. The diff coverage is 93.54%.

@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
- Coverage   98.19%   89.23%   -8.97%     
==========================================
  Files           3        3              
  Lines         111      130      +19     
==========================================
+ Hits          109      116       +7     
- Misses          2       14      +12     

Impacted Files	Coverage Δ
lib/hooks.js	89.83% <92.85%> (-10.17%)	:arrow_down:
lib/helpers.js	84.44% <93.33%> (-15.56%)	:arrow_down:
lib/index.js	96.15% <100.00%> (+4.15%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9f408f6...d41041c. Read the comment docs.

[weaverryan]

I'm stuck on one last detail for now: for some reason, in Windows only, asset.info.sourceFilename in reduceAssets() - https://github.com/shellscape/webpack-manifest-plugin/blob/master/lib/helpers.js#L28 - is always missing.

What's stranger is that on our CI - we use AppVeyor - it is not missing, and everything works fine. I have no idea why that would be the case :/.

[shellscape]

Are there any other environment differences on AppVeyor versus our Actions? Node versions, other installed windows weird things?

[weaverryan]

Hmm, i've actually just added the same Windows matrix to our PR, and it's working fine - https://github.com/symfony/webpack-encore/pull/921/checks#step:6:11 - that test relies on the same thing that is mysteriously missing here. We're executing Webpack in exactly the same way... 🤔

[weaverryan]

I've asked for help here: https://github.com/webpack/webpack/issues/12637

I absolutely had this issue cornered from all angles... until I hit the Windows bug in this CI only (which I can't repeat anywhere else, not even on my own CI). In general, what this plugin tries to do is simple: get a list of all files that were emitted and make a map from the "sourceFilename": "outputFilename" in JSON. On a high level, that should be simple to do. But this library has to do all kinds of tricks to do it: getting the .js and .css entry files from compilation.chunks, hooking into file-loader via the emitFile() callback, auxiliary files and (now) hooking into NormalModule.loader for asset modules.

Anyways, I'm hoping there is a simpler way. When asset.info.sourceFilename vanished from images on the Windows CI here only, that sort of broke all my plans and I have not found a way to make the map from "output filename" to "source filename" any other way.

If you have any ideas, I would love to hear them :).

Thanks!

[weaverryan]

This is now ready!!!!!!

I have updated the PR description. In short, a crazy npm+Windows bug caused an "invalid" problem, which the test suite now works around. I was also able to simplify the PR a bit after getting guidance from the Webpack folds.

Please let me know if you need anything else - I'm hoping (now that this is finally complete... and it almost killed me 😉 ) that we could release this soon (and then I can avoid releasing my "copy" of the plugin inside symfony/webpack-encore).

Thanks!

[aboks]

@weaverryan as we were having some issues with Webpack 5, webpack-manifest-plugin and copy-webpack-plugin (seemingly related to #237 and #238), I tried out your branch. It seems to fix some of the issues, but still handles one case incorrectly.

We have something in our Webpack config that looks like

new CopyWebpackPlugin({
    "patterns": [
        { "from": "./path/to/common/favicon.ico", "to": "favicon.ico" },
        { "from": "./path/to/a/favicon.ico", "to": "favicon_a.ico" },
        { "from": "./path/to/b/favicon.ico", "to": "favicon_b.ico" },
    ]
})

Under Webpack 4 (with webpack-manifest-plugin 2.2.0) this resulted in the following manifest entries. I assume these to be correct.

{
    "favicon.ico": "favicon.ico",
    "favicon_a.ico": "favicon_a.ico",
    "favicon_b.ico": "favicon_b.ico"
}

Under Webpack 5 (with webpack-manifest-plugin 3.0.0, without your fix) this resulted in the following entries:

{
    "path/to/common/favicon.ico": "favicon.ico",
    "path/to/a/favicon.ico": "favicon_a.ico",
    "path/to/b/favicon.ico": "favicon_b.ico"
}

Now with your fix we only get a single manifest entry for the favicons:

{
    "favicon.ico": "favicon_b.ico"
}

It seems like the logic to combine the dirname from one path with the basename of another from https://github.com/weaverryan/webpack-encore/commit/d83e6dbe43fbaa911e2364b24c14f77bcc331dab#diff-8a4026eedbd67006d86faab015b15c08372284c5c957cfd5a49e0faf8dd85396R34 does not work nicely when files copied using copy-webpack-plugin get a different name than they had originally, especially if the original names collide.

Would there be a way to change your fix to handle this situation correctly? I looked at a possible solution, but I have to admit I'm too unfamiliar with Webpack internals and asset modules to grasp what exactly is going on. Maybe it's necessary to distinguish the asset module case from the copied asset case using asset.info.contenthash or asset.info.copied?

[shellscape]

@aboks I'm going to merge this one now so folks get the benefit of the changes. We can iterate to solve that last case later on.

[shellscape]