shellscape / webpack-manifest-plugin

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

fix: Only include initial chunks #48

Closed a-x- closed 7 years ago

a-x- commented 7 years ago

closes #46 Do not add code splitting bundles into manifest

mastilver commented 7 years ago

Do we need the omitDlls options?

Couldn't you use chunk.initial or chunk.isInitial()?

a-x- commented 7 years ago

What is chunk.initial or chunk.isInitial()? I cannot find them in the source code or documentation

mastilver commented 7 years ago

it's a boolean on chunk that tell if they are required on initial run

.filter(chunk => chunk.isInitial ? chunk. isInitial() : chunk.initial

a-x- commented 7 years ago

Thank you,

As I understand isInitial might be a replacement for the chunk.entrypoints in that case.

But I'm not sure to make this new logic default for all users in the next minor version.

mastilver commented 7 years ago

I doubt it can be considered as a breaking change, to me it's definitely a fix, so it should land on a patch (BTW, some tests would be great! ;) )

a-x- commented 7 years ago

done

Ianfeather commented 7 years ago

@mastilver Hi! First of all thanks for your work on this project, we use it in the asset-pipeline at BuzzFeed.

Unfortunately this change broke one of our applications this morning as it was relying on the dynamic chunks being included in the asset-manifest. We use these manifest entries to preload bundles ahead of time with link[rel=preload]. We can probably find some alternative but it would be great if you would consider having this as an option. I think this is a very legitimate use case.

a-x- commented 7 years ago

Hey, we wanted to make it optional initially 😂

mastilver commented 7 years ago

@Ianfeather Damn, I was really hoping for my first release to go smoothly... :/

I will revert that change for now, we can think for another solution later

a-x- commented 7 years ago

@Ianfeather, you should use chunk-manifest-webpack-plugin. I'm sorry about this

my position, this change should be default but major @mastilver, what about you?

mastilver commented 7 years ago

@Ianfeather webpack-manifest-plugin@1.0.2 released :)

mastilver commented 7 years ago

@a-x- I thing the right path to take is: by default include every single possible assets into the manifest and be able to have some filtering, I will put my thought down on an issue over the week-end, I will be happy to get your thoughts on that ;)

a-x- commented 7 years ago

ok, I can restore initial flagged functionality

mastilver commented 7 years ago

@a-x- I want to work on something more modular, it will solve your issue and few others

In the meantime, I would love to get some help on #51 as I won't be merging anything else until that's done. that way I'm sure we don't introduce new breaking changes

Ianfeather commented 7 years ago

@mastilver Thanks for your quick turnaround on this! It's much appreciated.

@a-x- thanks for the link to that project. That will also help us remove our webpack-runtime script too