Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
skliffmueller wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/writeFile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,12 @@ export default function writeFile(globalRef, pattern, file) {
return;
}

// Used to announce these files as moduleAssets.
// 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

@alexander-akait alexander-akait Dec 4, 2018

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

@skliffmueller skliffmueller Dec 4, 2018

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

@alexander-akait alexander-akait Dec 4, 2018

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

@alexander-akait alexander-akait Dec 4, 2018

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

@skliffmueller skliffmueller Dec 4, 2018

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

@alexander-akait alexander-akait Dec 5, 2018

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

@skliffmueller skliffmueller Dec 5, 2018

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.


info(`writing '${file.webpackTo}' to compilation assets from '${file.absoluteFrom}'`);
compilation.assets[file.webpackTo] = {
size: function() {
Expand Down