Skip to content
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.

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

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

Closed
BernsteinA opened this issue Oct 30, 2017 · 15 comments · Fixed by #107
Milestone

Comments

@BernsteinA
Copy link
Contributor

BernsteinA commented Oct 30, 2017

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 mastilver added the bug label Oct 31, 2017
@mastilver
Copy link
Contributor

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

@mastilver mastilver added this to the v2.x milestone Oct 31, 2017
@BernsteinA
Copy link
Contributor Author

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
Copy link
Contributor

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

@mastilver
Copy link
Contributor

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
Copy link
Contributor Author

@mastilver
Copy link
Contributor

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-
Copy link
Contributor

a-x- commented Nov 4, 2017

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

@mastilver
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

BernsteinA commented Nov 6, 2017

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:

  • make it work with an array of configs
  • update the readme with an explanation
  • throw an error when a user tries to have multiple manifest plugins

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

@mastilver
Copy link
Contributor

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
Copy link

deguif commented Nov 15, 2017

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
Copy link
Contributor

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

@mastilver
Copy link
Contributor

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

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 a pull request may close this issue.

4 participants