Skip to content

Copy plugin integration #75

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

Closed
wants to merge 5 commits into from
Closed

Copy plugin integration #75

wants to merge 5 commits into from

Conversation

davidmpaz
Copy link
Contributor

Hi @mastilver,
This is the follow up of #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
Copy link

codecov-io commented Aug 23, 2017

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%) ⬆️

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

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

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

@mattandrews
Copy link

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

mastilver commented Aug 25, 2017

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

@mattandrews
Copy link

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

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

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

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:

  • I was using the wrong plugin in my initial attempts (I was using webpack-assets-manifest instead of this plugin – sorry, this was a sloppy mistake)

  • Despite this, when using @davidmpaz's fork of the plugin, I don't get the output I expect in my test repo above. It's probable I'm not using Webpack correctly (I'm still pretty new to it) – I'm coming from a gulp workflow so trying to copy my files to an output directory, version them, then generate a manifest that contains their input/output pairings. It seems as though the copy-webpack-plugin (which I appreciate you have no experience of, @mastilver!) doesn't pass the pre-hashed filename properly through to this fork?

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

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

davidmpaz commented Aug 29, 2017

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:

  • copy-webpack-plugin#105
  • copy-webpack-plugin#104

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 pushed a commit that referenced this pull request Sep 10, 2017
BREAKING CHANGE: add extra keys to manifest when using copy-webpack-plugin

closes #75
mastilver pushed a commit that referenced this pull request Sep 10, 2017
BREAKING CHANGE: add extra keys to manifest when using copy-webpack-plugin

closes #75
mastilver pushed a commit that referenced this pull request Sep 10, 2017
BREAKING CHANGE: add extra keys to manifest when using copy-webpack-plugin

closes #75
@mastilver mastilver closed this in 4cb95ce Sep 10, 2017
@mastilver
Copy link
Contributor

Thank you for your work @davidmpaz :)

@bxm156
Copy link

bxm156 commented Sep 27, 2017

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

Levdbas commented Oct 26, 2017

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

@Levdbas I'm afraid no... #75 (comment)

@mattandrews
Copy link

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

Levdbas commented Oct 26, 2017

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

deguif commented Mar 3, 2018

See my webpack-contrib/copy-webpack-plugin#104 (comment) for a working solution using copy-webpack-plugin.

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 this pull request may close these issues.

7 participants