shellscape / webpack-manifest-plugin

webpack plugin for generating asset manifests
MIT License
1.44k stars 184 forks source link

having multiple instances of this plugin seems to prevent webpack from emitting any files other than the manifests #100

Closed BernsteinA closed 6 years ago

BernsteinA commented 6 years ago

2.0.0-rc.1

    new ManifestPlugin({
        fileName: 'manifest-android.json',
        filter: (file)=>!file.path.match(/\.mp3$|\.map$/),
    }),
    new ManifestPlugin({
        fileName: 'manifest-ios.json',
        filter: (file)=>!file.path.match(/\.ogg$|\.map$/),
    })

with only one ManifestPlugin everything works. with two, no assets are emitted by webpack, except the two manifest jsons.

mastilver commented 6 years ago

Interesting... I wonder if it's due to: https://github.com/danethurber/webpack-manifest-plugin/blob/409b0fb64234ed9fa9adeeba31850a4d4feeca7c/lib/plugin.js#L173-L178

Could you send me a failing PR, please

BernsteinA commented 6 years ago

Not sure what you mean by a failing PR. Add a test to https://github.com/danethurber/webpack-manifest-plugin/blob/master/spec/plugin.integration.spec.js ?

mastilver commented 6 years ago

yes, or rather: spec/plugin.spec.js

mastilver commented 6 years ago

So I don't think I'm going to fix that

I believe it's bad practice to have multiple instances of the same plugins (I could be wrong)

But I think we can work around that: manifest-webpack-plugin could accept an array of options

 new ManifestPlugin([
    {
        fileName: 'manifest-android.json',
        filter: (file)=>!file.path.match(/\.mp3$|\.map$/),
    },
    {
        fileName: 'manifest-ios.json',
        filter: (file)=>!file.path.match(/\.ogg$|\.map$/),
    }
])

What do you think?

BernsteinA commented 6 years ago

I haven't seen anything that suggests it's bad practise to have multiple instances of the same plugin. https://github.com/jantimon/html-webpack-plugin#generating-multiple-html-files https://webpack.js.org/plugins/commons-chunk-plugin/#combining-implicit-common-vendor-chunks-and-manifest-file

mastilver commented 6 years ago

Thank you for the CommonsChunkPlugin example 👍

I'm one of the maintainer of html-webpack-plugin and issues with multiple instances are just a pain to deal with ;)

Anyway I will try to see if I can fix it (by allowing multiple instances), feel free to have a look on your side as well :)

a-x- commented 6 years ago

Why the new ManifestPlugin([ { fileName }, { fileName },,, ]) syntax isn't enough?

mastilver commented 6 years ago

The way to resolve that would be to have a lock per files rather than a lock per instance

@BernsteinA Do you think you can have a look at it?

mastilver commented 6 years ago

Why the new ManifestPlugin([ { fileName }, { fileName },,, ]) syntax isn't enough?

Not sure that's the right approach here to be honest... After @BernsteinA example, I think supporting multiple instance is the best / less confusing

mastilver commented 6 years ago

Actually... I just changed my mind, I don't think we should solve this issue... We should only be supporting array options. It's the easiest way path and it will cause less issues

If we ever removed the lock there would be another issue with multiple instances: a manifest file will reference the other one randomly, that's why I think we shouldn't fix that issue

On top of the API docs, we should add an FAQ with Can I create multiple manifest files? that will show the array options in more details

@BernsteinA Interested in working on that?

BernsteinA commented 6 years ago

I think as well as adding to the readme, we should detect when there are multiple instances of the plugin, and throw an error instead of the current behavior which times out instead of fails. So:

I can definitely contribute to these solutions but can't commit to a timeline. Pretty swamped with work right now

mastilver commented 6 years ago

we should detect when there are multiple instances of the plugin

Yep, we could use compiler.options.plugins example

Pretty swamped with work right now

No worries, I think most of us are like that atm, I understand :)

deguif commented 6 years ago

I was trying to generate a manifest for each type of resource (css, js, ...) and found this issue. Is there any ETA to communicate?

mastilver commented 6 years ago

@deguif No, not yet, I don't have that much time atm. But feel free to give it a go :)

mastilver commented 6 years ago

I got something working, I will try to push it tonight or tomorrow