shellscape / webpack-manifest-plugin

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

Copy webpack plugin integration #45

Closed davidmpaz closed 7 years ago

davidmpaz commented 7 years ago

Hi,

first of all, thanks for this project. I am writing to ask whether could be possible to include this patch in main stream or in case is not good, lets try to find/coordinate a solution for include copied asset in manifest.

My use case is that I have several images used in php templates, these are not compiled/processed by Webpack and as such they are never copied to target directory.

In this scenario, plugins like copy-webpack-plugin are very helpful. One missing feature, I think many people will appreciate, is to have those copied assets in the manifest file also.

Some notes on my findings:

Many thanks in advance and best regards.

weaverryan commented 7 years ago

@davidmpaz could you add a test for this? Does it work in all the situations you've found? Or are there some caveats?

mastilver commented 7 years ago

I'm sorry you need to find another solution for that, it will output 'normal' file twice

I'm also not sure about the option, ideally I would like to land this on v2 turned on by default

mastilver commented 7 years ago

@davidmpaz If you test again the latest master and includeCopiedAssets on, tests will fail

davidmpaz commented 7 years ago

Hi @mastilver precisely today I was trying to add some tests and figure it out of that duplication with tests. I should have had added tests since the beginning. Test are failing in travis now but I will fix them soon. Today I can not continue.

@weaverryan I don't see any other caveats besides the issue with duplication mentioned, which should be fixed now.

mastilver commented 7 years ago

@davidmpaz No problem at all :)

Take your time, I'm planning to merge your PR for v2 which is not happening anytime soon I will review your existing work. Pick it up only when you are ready ;)

codecov-io commented 7 years ago

Codecov Report

Merging #45 into master will increase coverage by 0.47%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
+ Coverage   92.85%   93.33%   +0.47%     
==========================================
  Files           2        2              
  Lines          56       60       +4     
==========================================
+ Hits           52       56       +4     
  Misses          4        4
Impacted Files Coverage Δ
lib/plugin.js 93.22% <100%> (+0.49%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a3d3a8e...59d4bb9. Read the comment docs.

mastilver commented 7 years ago

Hi @davidmpaz

Can you let me know if you have time to carry on with this one, I want to release 2.x next week-end

Can you remove all your style changes: no space between function and parentheses

Thank you :)

davidmpaz commented 7 years ago

Hi @mastilver,

sure. I did notice that you merged to update branches but thought you were going to continue working on it.

I will take a look to finish it between today and tomorrow, otherwise I will not have time later :)

cheers

mastilver commented 7 years ago

Great, thank you :)

Let me know if you can't so I can carry on

One last thing, for those files, can you set isModuleAsset to false, please, that way there is a way to tell them apart

davidmpaz commented 7 years ago

Hi @mastilver,

after looking better to current work I decided would be easier to just create another pull request than re-basing this one. The changes have been to much to conciliate again what was there :)

Please lets continue in another PR I will made now.