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

Add a copyFiles() method to the public API (using CopyWebpackPlugin) #221

Closed
wants to merge 1 commit into from

Conversation

Lyrkan
Copy link
Collaborator

@Lyrkan Lyrkan commented Dec 4, 2017

Note: Can't be merged yet as it needs #164 for the tests to pass. Right now everything should work apart from the copied files not being added to the manifest.json (see shellscape/webpack-manifest-plugin#75).

This PR adds a copyFiles() method to the API (closes #175):

Encore.copyFiles([
    'file.txt'
     { from: 'from/file.txt' },
     { from: 'from/file.txt', to: 'to/file.txt' },
     { from: 'from/file.txt', to: 'to/directory' },
     { from: 'from/directory' },
], (options) => {
    options.ignore = ['*.txt']
});

Multiple calls will add new instances of the plugin, each one being configured using the patterns and callback it has been added with.

@Lyrkan Lyrkan added the Feature New Feature label Dec 4, 2017
@weaverryan
Copy link
Member

We're just waiting on the stable release: https://github.com/danethurber/webpack-manifest-plugin/releases

Technically, I believe we could use the RC1 as a package and NOT wait for stable... but hopefully it'll happen soon - I pinged the maintainers.

@JohanLopes
Copy link

@weaverryan @Lyrkan The new version of Webpack manifest version has been released : https://github.com/danethurber/webpack-manifest-plugin/releases

@Lyrkan
Copy link
Collaborator Author

Lyrkan commented Apr 11, 2018

@JohanLopes Yep, waiting for #164 to be updated and merged before rebasing that one :)

@weaverryan
Copy link
Member

Ok, #164 is merged! go go go! :)

@Lyrkan
Copy link
Collaborator Author

Lyrkan commented Apr 14, 2018

@weaverryan I rebased it but I'm a bit annoyed by some of the test results:

Encore.copyFiles([{ from: 'images/symfony_logo.png', to: 'symfony_logo.[hash:8].png' }]);

// Expected manifest.json
{"build/symfony_logo.png": "/build/symfony_logo.ea1ca6f7.png"}

// Actual manifest.json
{"build/symfony_logo.ea1ca6f7.png": "/build/symfony_logo.ea1ca6f7.png"}

I'm not sure what to do about that...

Also (but less of an issue so I didn't include it in my tests):

Encore.copyFiles([{ from: 'images/symfony_logo.png', to: './symfony_logo.png' }]);

// Expected manifest.json
{"build/symfony_logo.png": "/build/symfony_logo.png"}

// Actual manifest.json
{"build/./symfony_logo.png": "/./build/symfony_logo.png"}

@Lyrkan
Copy link
Collaborator Author

Lyrkan commented Apr 17, 2018

I just noticed that the first issue was already discussed there: shellscape/webpack-manifest-plugin#75 (comment)

So... nothing can really be done for now since the Copy Webpack Plugin doesn't expose the original filename (that PR could help though: webpack-contrib/copy-webpack-plugin#198).

@weaverryan
Copy link
Member

Boooooooooooo

Just to throw out an idea... if we copied that plugin (yet again) to our code, are there some small changes that would allow us to have this feature? It's so important, I'd rather be pragmatic over pure.

@weaverryan
Copy link
Member

Oh, and since you understand the problem well, I think you should give some good feedback on /webpack-contrib/copy-webpack-plugin#198 if it's the right approach (more than just 👍, mention why we need it & your issue - the maintainer is good about listening when somebody comes in with good technical knowledge).

@deguif
Copy link

deguif commented Jun 1, 2018

Just seing this conversation.

I found a way to bypass this issue a little time ago, having a look at the following comment can help with solving this problem: webpack-contrib/copy-webpack-plugin#104 (comment)

The bypass is not very clean but it allows to generate a manifest with proper key when using hash.

@Lyrkan
Copy link
Collaborator Author

Lyrkan commented Jun 1, 2018

@deguif I saw your comment but sadly I don't think this is generic enough to add into this PR (it would not work for some filenames/configurations).

I'm also not sure that webpack-contrib/copy-webpack-plugin#198 would be the right fix anymore... as some other people pointed out in that thread an event-based approach would probably be a better idea.

@deguif
Copy link

deguif commented Jun 1, 2018

Clearly I don't think webpack-contrib/copy-webpack-plugin#198 is the right fix, it would also be redundant with the use of webpack manifest plugin.

@weaverryan
Copy link
Member

I haven’t looked at the specifics, but if there is a workaround that would work for us, but not be exposed to our users (so if we change in the future, they won’t be affected), then we should check into it. It’s tough, this is such an important feature but one that has been continually blocked.

@Lyrkan
Copy link
Collaborator Author

Lyrkan commented Jul 19, 2018

@weaverryan I looked again at this PR today and I don't think there is a viable workaround.

Imagine that you have the following config (which is more or less the one I used for the failing functional test):

Encore.copyFiles([
  { from: 'images/symfony_logo.png', to: 'symfony_logo1.png' },
  { from: 'images/symfony_logo.png', to: 'symfony_logo2.png' },
  { from: 'images/symfony_logo.png', to: 'symfony_logo3.[hash:8].png' },
]);

What do you expect to get in the manifest file? Maybe something like this (which is what @deguif 's solution does if you only use the [name].[hash].[ext] pattern):

{
  'build/symfony_logo.png': '/build/symfony_logo1.png',
  'build/symfony_logo2.png': '/build/symfony_logo2.png',
  'build/symfony_logo3.png': '/build/symfony_logo3.ea1ca6f7.png'
}

But no... the key should actually always be build/symfony_logo.png because the left part is supposed to be the original name of the file (you can't guess what you're supposed to get from the final filename). This is annoying because currently the generated manifest would look like this:

{
  'build/symfony_logo1.png': '/build/symfony_logo1.png',
  'build/symfony_logo2.png': '/build/symfony_logo2.png',
  'build/symfony_logo3.ea1ca6f7.png': '/build/symfony_logo3.ea1ca6f7.png'
}

... which doesn't contain the original filename at all.

So, unless the CopyWebpackPlugin provides a way to get more info there isn't much we (or the WebpackManifestPlugin) can do... and then you'd probably still have some edge cases to handle.

I'm wondering if we couldn't do something simpler without the CopyWebpackPlugin... like creating a fake entry point that require the given files using the file-loader.

@Lyrkan Lyrkan changed the title Add a copyFiles() method to the public API Add a copyFiles() method to the public API (using CopyWebpackPlugin) Oct 13, 2018
weaverryan added a commit that referenced this pull request Oct 19, 2018
…e.context) (Lyrkan)

This PR was squashed before being merged into the master branch (closes #409).

Discussion
----------

Add a copyFiles() method to the public API (using require.context)

This PR is an alternative solution to #221 since that one is still blocked because of the `CopyWebpackPlugin`/`WebpackManifestPlugin` issue.

Instead of using an external plugin it relies on a fake entry that basically does multiple `require.context()` calls and forces them to use the `file-loader`.

The API is a bit different, and may be a bit harder to use since globs are replaced by RegExps.

```javascript
// Copy the content of a whole directory and its subdirectories
Encore.copyFiles({ from: './images' });

// Only copy files matching a given pattern
Encore.copyFiles({ from: './images', pattern: /\.(png|jpg|jpeg)$/ })

// Set the path the files are copied to
Encore.copyFiles({
	from: './images',
	pattern: /\.(png|jpg|jpeg)$/,
	to: 'assets/images/[path][name].[ext]'
})

// Version files
Encore.copyFiles(
	from: './images',
	to: 'assets/images/[path][name].[hash:8].[ext]'
})

// Add multiple configs in a single call
Encore.copyFiles([
	{ from: './images' },
	{ from: './txt', pattern: /\.txt$/ },
]);
```

Commits
-------

a9ae08d Use versioned filenames during copying if the "to" option isn't set
7878f4e Add Encore.copyFiles() method to the public API
@Lyrkan
Copy link
Collaborator Author

Lyrkan commented Oct 20, 2018

Closing that one since #409 has been merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a copyFiles() Method
4 participants