Skip to content

Support for manifest plugin and other modules utilizing moduleAsset hook#309

Closed
skliffmueller wants to merge 1 commit intowebpack:masterfrom
skliffmueller:master
Closed

Support for manifest plugin and other modules utilizing moduleAsset hook#309
skliffmueller wants to merge 1 commit intowebpack:masterfrom
skliffmueller:master

Conversation

@skliffmueller
Copy link

It's stupid simple. Announce to moduleAsset hook. Now original reference name links to new asset name. I'm sure this will help resolve other related conflicts with plugins and post processing against asset paths.

Issue #104

@jsf-clabot
Copy link

jsf-clabot commented Dec 3, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


skliffmueller seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need tests

// This allows the paths to be resolved by things like the manifest plugin
compilation.hooks.moduleAsset.call({
userRequest: file.relativeFrom
}, file.webpackTo);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not sure it is right solution, because we interpreted files as module assets

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works for blizzard websites. And I don't see why this wouldn't work as this is how manifest plugin knows if dist path names have changed. I'll figure out test cases if I have time, otherwise it is a working solution.
https://github.com/danethurber/webpack-manifest-plugin/blob/master/lib/plugin.js#L86-L87
https://github.com/danethurber/webpack-manifest-plugin/blob/master/lib/plugin.js#L214-L216
https://github.com/danethurber/webpack-manifest-plugin/blob/master/lib/plugin.js#L42-L49

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skliffmueller Here (https://github.com/danethurber/webpack-manifest-plugin/blob/master/lib/plugin.js#L59) we need implement hook compilation.applyPluginsAsync('webpack-manifest-plugin-before-emit', callback);

And use this hook in this plugin.

We should not pass this as module because it can break analyze tools.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And to avoid problems with other plugins we need implement new hooks in webpack, example manifest which call after emit. It is also easy implement and feel free to ping me in webpack repo

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that will help the situation, as copy-webpack-plugin doesn't need to know what the manifest looks like, webpack-manifest-plugin needs to know what copy-webpack-plugin has modified. In webpack-manifest-plugin stats.assets from the complication object is the only point where the copy-webpack-plugin assets exist. and moduleAssets is the only object map to reference the hash file names to the original path. Otherwise it defaults to the file name.
https://github.com/danethurber/webpack-manifest-plugin/blob/master/lib/plugin.js#L88-L114

So either copy-webpack-plugin is not implemented correctly, webpack complication assets is not implemented correctly (needs to include ability to set asset name and path), or webpack-manifest-plugin isn't implemented correctly and needs to listen to an additional hook to build against the object mapping between hash file names and original file names.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to listen to an additional hook to build against the object mapping between hash file names and original file names.

Yes, because adding this as module can break analyze tools, also it is not module.

Manifest plugin have 2 problem:

  1. Using emit hooks (it is not safe because other plugins also add assets on this hook and some assets may disappear). As i said above we need new hook in webpack for this. You can send a PR in webpack, I will support you, should be easy.
  2. Plugin should add hook for adding assets (I showed the place where it should be).

These tasks are in my todo lists, but at the moment there are more priorities, so you can start doing this and I will be happy to help.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I might not have clearly understood you at first. But I get it, we are on the same page. I will look into doing this over the weekend when work isn't chasing after me.

@alexander-akait
Copy link
Member

alexander-akait commented Nov 3, 2020

Fixed in master, now we have sourceFilename in assets info, so you can get easy get original filename, maybe you need to pen an issue for supporting it in the plugin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants