Fixing extra and invalid paths in Webpack 5 #249

weaverryan · 2021-02-07T19:05:18Z

This PR contains:

Are tests included?

yes
[] no

Breaking Changes?

yes (breaking changes will not be merged unless absolutely necessary)
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 symfony/webpack-encore#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 - webpack/webpack#12637 (comment) - 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 · 2021-02-08T15:42:12Z

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 2702efb 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 · 2021-02-09T02:49:10Z

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%)`	⬇️
lib/helpers.js	`84.44% <93.33%> (-15.56%)`	⬇️
lib/index.js	`96.15% <100.00%> (+4.15%)`	⬆️

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.

shellscape · 2021-02-09T02:51:08Z

lib/helpers.js

-const reduceAssets = (files, asset, moduleAssets) => {
-  const name = moduleAssets[asset.name] ? moduleAssets[asset.name] : asset.info.sourceFilename;
+const reduceAssets = (files, asset, moduleAssets, assetTypeModuleAssets) => {
+  console.log(assetTypeModuleAssets, asset.info.sourceFilename);


This little guy snuck into the party

Oh yea :) I'm actively debugging a last Windows issue that I can only replicate on this CI. So i'm abusing the Windows CI (I don't have access to any other Windows machine).

I'll let you know when things are ready to look at - I thought I had it so I must be missing a tiny last detail :)

weaverryan · 2021-02-09T02:25:48Z

lib/helpers.js

+    };
+  });
+
+  return Array.from(chunk.files).reduce((prev, path) => {


If you view this section without whitespace - https://github.com/shellscape/webpack-manifest-plugin/pull/249/files?w=1 - you'll see that the existing logic remains the same. The chunk.auxiliaryFiles were removed from the "normal" logic and moved into the above loop to track those custom.

weaverryan · 2021-02-09T02:27:41Z

lib/helpers.js

+      isInitial: false,
+      isChunk: false,
+      isAsset: true,
+      isModuleAsset: true


I believe these are all correct: these are not chunks, they are assets. And I believe they are "module assets" because they are auxiliary assets to a chunk, but I'm not positive if that is the intention. These will most notably include sourcemap files.

weaverryan · 2021-02-09T02:33:14Z

lib/hooks.js

+    moduleAssets,
+    assetTypeModuleAssets,
+    options
+  },


A few of these cs changes are being made automatically by the linter - I tried to keep the diff as clean as possible :)

weaverryan · 2021-02-09T02:35:59Z

test/integration/location.js

@@ -22,12 +22,12 @@ test('output to the correct location', async (t) => {
      filename: '[name].js',
      path: outputPath
    },
-    plugins: [new WebpackManifestPlugin({ fileName: 'webpack.manifest.js' })]
+    plugins: [new WebpackManifestPlugin({ fileName: 'webpack.manifest.json' })]


If you end the file in .js, the TerserPlugin tries to parse these files and explodes. I'm not sure why this is different between v4 and v5 (it's only a problem on v5), but it makes sense: the manifest contents are not actually a valid JavaScript file ({ "foo": "bar" } is not valid JavaScript - you'd need to have a const = in front or something equal).

And so, I think this is valid to change. Users are able to control the filename (the point of the test), but in Webpack 5, they cannot name it .js.

Is that something we should document in the README?

Hmm. I would say, let's wait to see if we have any questions about it. The only reason I'm hesitating is that the JSON was never a valid JavaScript file... so I'm trying to think of what the use-case ever would have been (and so, is anyone out there actually doing that?).

weaverryan · 2021-02-09T02:36:13Z

test/unit/index.js

@@ -75,15 +74,15 @@ test('works with source maps', async (t) => {
      one: '../fixtures/file.js'
    },
    output: {
-      filename: '[name].js',
+      filename: 'build/[name].js',


Just making the test more interesting.

weaverryan · 2021-02-09T02:37:01Z

test/unit/index.js

-  // Note: I believe this to be another bug in webpack v5 and cannot find a good workaround atm
-  if (webpack.version.startsWith('5')) {
-    expected['main.txt'] = 'file.txt';
-  }


It turns out, these are now fixed! They are #237

weaverryan · 2021-02-09T02:53:20Z

lib/helpers.js

-const reduceAssets = (files, asset, moduleAssets) => {
-  const name = moduleAssets[asset.name] ? moduleAssets[asset.name] : asset.info.sourceFilename;
+const reduceAssets = (files, asset, moduleAssets, assetTypeModuleAssets) => {
+  console.log(assetTypeModuleAssets, asset.info.sourceFilename);


Oh yea :) I'm actively debugging a last Windows issue that I can only replicate on this CI. So i'm abusing the Windows CI (I don't have access to any other Windows machine).

I'll let you know when things are ready to look at - I thought I had it so I must be missing a tiny last detail :)

weaverryan · 2021-02-09T03:12:15Z

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 · 2021-02-09T04:03:21Z

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

weaverryan · 2021-02-09T13:49:33Z

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 · 2021-02-09T19:28:10Z

I've asked for help here: webpack/webpack#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 · 2021-02-09T22:23:31Z

lib/helpers.js

+  if (moduleAssets[asset.name]) {
+    name = moduleAssets[asset.name];
+  } else if (asset.info.sourceFilename) {
+    name = join(dirname(asset.name), basename(asset.info.sourceFilename));


This is basically the same thing that's already being done at the bottom of hooks.js for the file-loader/ NormalModule situation -

webpack-manifest-plugin/lib/hooks.js

Line 122 in 9f408f6

Object.assign(moduleAssets, { [file]: join(dirname(file), basename(module.userRequest)) });

weaverryan · 2021-02-09T22:26:14Z

set-webpack-version.js

+ * (because npm 6 does not do that but npm 7 does) is not enough. For some
+ * reason why must, by hand, update the package.json file first and then
+ * run a normal "npm install".
+ */


This is insanity, but THIS turned out to be the reason that asset.info.sourceFilename was missing on Windows (and why I could repeat it). I know from the Webpack folks - webpack/webpack#12637 (comment) - that relying on asset.info.sourceFilename IS the correct way to do things.

Basically, for reasons that aren't clear, on Windows only, if you installed Webpack 5 via npm install webpack -D it would not work. Somewhere, internally, some dependencies must have gotten "crossed". But if you manually change the peerDependencies and devDependencies for Webpack and then run npm install, things work fine. This... almost killed me. But it turns out that this was an edge-case npm + Windows bug of some sort... or possibly some edge case since the package.json file is mixed with libraries that have Webpack 4 vs Webpack 5 as their peer dependency.

This is fascinating. What we need to know before we merge is why this was an issue only now. We've been testing webpack v4 and webpack v5 separately with the windows workflows without issue up to this point. So what's the missing bit of nuance as to why it's an issue now?

@weaverryan ping on this comment

weaverryan · 2021-02-09T22:27:03Z

test/unit/index.js

+    t.truthy(manifest);
+    t.deepEqual(Object.keys(expected), ['main.js', 'images/manifest.svg']);
+    t.deepEqual(manifest['main.js'], 'main.js');
+    t.regex(manifest['images/manifest.svg'], /images\/manifest\.[a-z|\d]{4}\.svg/);


Doing this so I could do a fuzzy check on the hash. The [fullhash] thing does not work for assetModuleFilename, so I needed to use a real hash.

weaverryan · 2021-02-09T22:28:45Z

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!

…fest-plugin fix) (weaverryan) This PR was squashed before being merged into the main branch. Discussion ---------- Fixing manifest paths (by temporarily embedding webpack-manifest-plugin fix) This temporarily embeds shellscape/webpack-manifest-plugin#249, which fixes #907 and also a comment on #914 (comment) To test this: ``` $ yarn remove @symfony/webpack-encore $ yarn add git://github.com/weaverryan/webpack-encore.git#try-manifest-fix ``` Please let me know if this does or doesn't fix your `manifest.json` file. I'm also interested in hearing from Windows users, as the fix requires some "path" manipulation. Thanks! Commits ------- b81428d Fixing manifest paths (by temporarily embedding webpack-manifest-plugin fix)

aboks · 2021-03-31T11:03:31Z

@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 weaverryan/webpack-encore@d83e6db#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 · 2021-04-01T13:55:16Z

@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 · 2021-04-01T13:55:45Z

This was referenced Feb 7, 2021

Manifest keys contain source path #238

Closed

Manifest populates main entry point with unrelated files with webpack 5 #237

Closed

Fixing manifest paths (by temporarily embedding webpack-manifest-plugin fix) symfony/webpack-encore#921

Merged

weaverryan added a commit to weaverryan/webpack-encore that referenced this pull request Feb 8, 2021

embedding fix: shellscape/webpack-manifest-plugin#249

def85a4

weaverryan force-pushed the fix-invalid-paths-webpack5 branch 4 times, most recently from 1afd6fd to 329988e Compare February 9, 2021 02:31

Fixing extra and invalid paths in Webpack 5

9021a8c

weaverryan force-pushed the fix-invalid-paths-webpack5 branch from 329988e to 9021a8c Compare February 9, 2021 02:43

shellscape reviewed Feb 9, 2021

View reviewed changes

weaverryan force-pushed the fix-invalid-paths-webpack5 branch from 00685c9 to fd755f5 Compare February 9, 2021 02:52

weaverryan commented Feb 9, 2021

View reviewed changes

weaverryan force-pushed the fix-invalid-paths-webpack5 branch 3 times, most recently from d41041c to e3dce2a Compare February 9, 2021 03:06

weaverryan added a commit to weaverryan/webpack-encore that referenced this pull request Feb 9, 2021

embedding fix: shellscape/webpack-manifest-plugin#249

93b8acc

weaverryan added a commit to weaverryan/webpack-encore that referenced this pull request Feb 9, 2021

embedding fix: shellscape/webpack-manifest-plugin#249

710b364

weaverryan force-pushed the fix-invalid-paths-webpack5 branch from 94b3587 to 2ebfba9 Compare February 9, 2021 22:01

trying the npm fix

185954d

weaverryan force-pushed the fix-invalid-paths-webpack5 branch from 2ebfba9 to 185954d Compare February 9, 2021 22:04

weaverryan added 2 commits February 9, 2021 17:13

allowing for any hash

44ad11c

removing all the extra logic

9e80c0d

weaverryan commented Feb 9, 2021

View reviewed changes

ureimers mentioned this pull request Feb 10, 2021

Version 1.0 problems in manifest.json when importing from composer vendor folder symfony/webpack-encore#907

Closed

weaverryan added a commit to weaverryan/webpack-encore that referenced this pull request Feb 11, 2021

embedding fix: shellscape/webpack-manifest-plugin#249

d83e6db

shellscape merged commit ef1f875 into shellscape:master Apr 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing extra and invalid paths in Webpack 5 #249

Fixing extra and invalid paths in Webpack 5 #249

weaverryan commented Feb 7, 2021 •

edited

Loading

shellscape commented Feb 8, 2021

codecov-io commented Feb 9, 2021 •

edited

Loading

shellscape Feb 9, 2021

weaverryan Feb 9, 2021

weaverryan Feb 9, 2021

weaverryan Feb 9, 2021

weaverryan Feb 9, 2021

weaverryan Feb 9, 2021

shellscape Feb 9, 2021

weaverryan Feb 9, 2021

weaverryan Feb 9, 2021

weaverryan Feb 9, 2021

weaverryan Feb 9, 2021

weaverryan commented Feb 9, 2021

shellscape commented Feb 9, 2021

weaverryan commented Feb 9, 2021

weaverryan commented Feb 9, 2021

weaverryan Feb 9, 2021

weaverryan Feb 9, 2021

shellscape Feb 12, 2021

shellscape Mar 18, 2021

weaverryan Feb 9, 2021

weaverryan commented Feb 9, 2021

aboks commented Mar 31, 2021

shellscape commented Apr 1, 2021

shellscape commented Apr 1, 2021

Fixing extra and invalid paths in Webpack 5 #249

Fixing extra and invalid paths in Webpack 5 #249

Conversation

weaverryan commented Feb 7, 2021 • edited Loading

Description

To test in your project

shellscape commented Feb 8, 2021

codecov-io commented Feb 9, 2021 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Feb 9, 2021

shellscape commented Feb 9, 2021

weaverryan commented Feb 9, 2021

weaverryan commented Feb 9, 2021

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

weaverryan commented Feb 9, 2021

aboks commented Mar 31, 2021

shellscape commented Apr 1, 2021

shellscape commented Apr 1, 2021

weaverryan commented Feb 7, 2021 •

edited

Loading

codecov-io commented Feb 9, 2021 •

edited

Loading