shellscape / webpack-manifest-plugin

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

Copy plugin integration #75

Closed davidmpaz closed 6 years ago

davidmpaz commented 7 years ago

Hi @mastilver, This is the follow up of danethurber/webpack-manifest-plugin#45. Unfortunately I will not have more time these days until next week for the project.

I need help here also with these lines, in the function existAssetInFiles. From tests I was getting an edge case for files ending in: .hot-update.json. Currently the solution I found was that you see there. But honestly I dont know from where that comes and whether this is the right way to check if the asset was included already. Please could you take a look and improve from there?

Things we can have in mind:

If there is something we could do from copy-webpack-plugin side to make easier that check, I can make a pull request to them. If you look at the mock plugin what it does is asimilar to what copy-webpack-plugin does. Maybe we can add another data there to use later in here to filter easily. Some flag that does not interfere with standard things and so on like:

compilation.assets['third.party.js'] = {
       copied: true, // this is what I mean as flag, though dunno if correct.
       size: function () {
         return compiledMock.length;
       },
       source: function () {
         return compiledMock;
       }
}

So in here we can do something like:

if (asset.copied) {
......
}

Sorry for the long post. regards

codecov-io commented 7 years ago

Codecov Report

Merging #75 into master will increase coverage by 0.14%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage   98.57%   98.71%   +0.14%     
==========================================
  Files           2        2              
  Lines          70       78       +8     
==========================================
+ Hits           69       77       +8     
  Misses          1        1
Impacted Files Coverage Δ
lib/plugin.js 98.7% <100%> (+0.15%) :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 04916cd...968dec1. Read the comment docs.

mastilver commented 7 years ago

Great thank you, I will have a look at it later

Sorry, if it was a bit painful due to all the changes :)

mattandrews commented 7 years ago

This is a feature I need – I've tried using this branch in my package.json but I'm still not seeing copied files appear in my manifest. This is my config:

plugins: [
        new CopyWebpackPlugin([{
            from: './images',
            to: './images',
            context: path.resolve(__dirname, 'assets')
        }]),
        new WebpackAssetsManifest({
            output: 'manifest.json',
            writeToDisk: true
        })
    ]

... manifest.json has my JavaScript files correctly listed, but not the images copied in CopyWebpackPlugin. Looking at these changes, it doesn't seem like I need to pass any additional options, but am I using it wrongly? I should expect to see the copied image files in my manifest file here, right?

mastilver commented 7 years ago

@mattandrews Can you double check you have this "webpack-manifest-plugin": "davidmpaz/webpack-manifest-plugin#copy-plugin" in your package.json

mattandrews commented 7 years ago

Hi @mastilver – this is what I have:

"webpack-manifest-plugin": "git://github.com/davidmpaz/webpack-manifest-plugin.git#copy-plugin"

When I browse the files it installs in node_modules, I can confirm it has the changes in this PR (eg. the existAssetInFiles function, etc).

mastilver commented 7 years ago

I think you will have to check with @davidmpaz, I have zero experience with that plugin..., but it's an issue if it doesn't work. Can you give us your config or better a repository to reproduce it

davidmpaz commented 7 years ago

Would be nice to have some more data to look at. Just tested again the branch and I see no problem.

Can you provide requested info @mattandrews ?

mattandrews commented 7 years ago

Thanks both – I've set up a simple repo here which shows an issue: https://github.com/mattandrews/webpack-manifest-copy-images-bug

A couple of things to note though:

To be honest this issue may be down to my misunderstanding of Webpack (or just a bad workflow I'm trying to create?) rather than an initial bug with this fork – certainly my report above was wrong because I wasn't even using this plugin 😞).

mastilver commented 7 years ago

@mattandrews I'm afraid this can't really be solved... unless you can have access to the written variable? https://github.com/kevlened/copy-webpack-plugin/blob/de4ff3231646548e9b524cde45cc1655cbb3373d/src/writeFile.js#L73

davidmpaz commented 7 years ago

Hi @mattandrews

don't worry too much about it, we all learn new stuff everyday :) Like @mastilver said is a bit complicate because of how copy-plugin works. From my experience, getting files copied with hashed file names for versioning was not working as expected.

One thing I did tried was to hash files when copying everything just one level under directory to copy from. That worked fine. Still when trying to use with globs could not make it work, honestly did not look much more deeper later on.

Some threads you could read, maybe can help to find a solution regarding versioning copied files:

Regarding manifest plugin, just be sure to add the copy-webpack-plugin before the manifest plugin in order to get all copied files included in the webpack compilation.

cheers

mastilver commented 6 years ago

Thank you for your work @davidmpaz :)

bxm156 commented 6 years ago

This worked great for me. I needed it to copy django's static files into webpack to get them into my build process and manifest.json Much appreciated!

Levdbas commented 6 years ago

Currently on the RC2. Does this also work with hashed filenames? Currently using the CopyWebPackPlugin like this:

new CopyWebpackPlugin([{ from: 'assets/images/', to: process.env.NODE_ENV === 'production' ? 'images/[name].[hash].[ext]' : 'images/[name].[ext]' }]

Expected output in manifest:

"images/file.jpg": "images/file.bf95ed4f733b97f45d2af4456872c2c3.jpg",

what gets outputted in the manifest:

"images/file.bf95ed4f733b97f45d2af4456872c2c3.jpg": "images/file.bf95ed4f733b97f45d2af4456872c2c3.jpg",

Am I missing something?

mastilver commented 6 years ago

@Levdbas I'm afraid no... https://github.com/danethurber/webpack-manifest-plugin/pull/75#issuecomment-325614092

mattandrews commented 6 years ago

@Levdbas I ended up not using this plugin and used a gulp task to version all my assets at build time (eg. for deployments, not day-to-day development). It simplified things as local development doesn't need versioning, and therefore I didn't need to tell Webpack about a bunch of unrelated files (image assets etc).

Levdbas commented 6 years ago

Thanks @mattandrews for your approach. Will consider someting like that as well if this really cant be solved.

@mastilver could the guys from copywebpack or any other plugin implement this new hook in such a way that no matter if the filename is hashed or not, it still gets a properly output in the manifest? In that case I will head over to the copywebpack repo to ask :)

deguif commented 6 years ago

See my https://github.com/webpack-contrib/copy-webpack-plugin/issues/104#issuecomment-370174211 for a working solution using copy-webpack-plugin.