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 require.context) #409

Merged
merged 2 commits into from
Oct 19, 2018

Conversation

Lyrkan
Copy link
Collaborator

@Lyrkan Lyrkan commented Oct 13, 2018

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.

// 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$/ },
]);

@Lyrkan Lyrkan added the Feature New Feature label Oct 13, 2018
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome! Very advanced / clever. The final result looks very good!

);
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


if (webpackConfig.copyFilesConfigs.length > 0) {
entries.push(copyEntryTmpName);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a super important detail, but I wonder if this will play nice with entrypoints.json - i.e. not have this fake entry there after #410. I suppose our "shared entry" already isn't a problem? So we'd be good here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I started the PR before #410 was created.
A check will have to be added there to exclude the temp file.

index.js Outdated
* copying a CSS file won't minify it)
* * By default files won't be versioned even if enableVersioning()
* has been called. In order to add a hash to your filenames you
* must use the "[hash]" placeholder in the "to" option (see below)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part surprised me. Though, I can see now (I think) why this is the case: we're parsing everything through file-loader, and the user is in control of their filename. 2 questions:

  1. Without ever writing any code, I was expecting the implementation of this would be basically to loop over and require each file that you want copied. In that case, they would be versioned automatically. But I guess, the problem is that you don't have control over the final filename / path?

  2. Could we (or is it a bad idea) hack the [hash] in when it's versioned? Or, do this if to is not specified, but make it the responsibility of the user if it is specified?

Copy link
Collaborator Author

@Lyrkan Lyrkan Oct 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the issue comes from the way the different calls to Encore.copyFiles are currently saved.

If you look at the copyFiles function in WebpackConfig.js you'll see that the to property of the defaultConfig object contains the default pattern ([path][name].[ext]). Unless the user explicitly sets to when adding a config it'll be this pattern that will be used when generating the related file-loader rule.

Since we don't know at that point if the versioning is enabled it's not possible to replace it by a versioned pattern.

That being said, if you want it to works that way I can probably do it quite easily by setting the to option to null in the default config. And then choose which pattern should be used only when generating the require calls.

);

webpackAssert.assertManifestPath(
'build/images/symfony_logo.png',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so to use this stuff, people would:

<img src="{{ path('build/images/symfony_logo.png') }}">

Right? They would basically "know" that the copy function puts things into the build/ directory, so then they reference that path. Gosh, and the manifest.json is even smart enough to create the key without the [hash] part of the filename? That's incredible!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it's not different than doing a require(...) inside of a real entry after all :)

It doesn't really remove the [hash] part but simply uses the original name of the file which is something that wasn't possible to do with the CopyWebpackPlugin since it didn't expose it.

@Lyrkan
Copy link
Collaborator Author

Lyrkan commented Oct 19, 2018

@weaverryan There you go, it should now work as expected when the to option isn't set and versioning is enabled :)

@weaverryan
Copy link
Member

Thank you @Lyrkan! This is awesome! All this work to update to Webpack 4... and I'm pretty sure this will be people's favorite feature of the next release :p

@weaverryan weaverryan merged commit a9ae08d into symfony:master Oct 19, 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
@ipernet
Copy link

ipernet commented Nov 16, 2018

Hi @Lyrkan

Thanks for .copyFiles(), it really well replaces hacking around CopyWebPackPlugin + StatsWriterPlugin to achieve the same result, but in a much cleaner way.

@piotr-cz
Copy link

the copyFiles expression lost ability to use globs in from paths as described in #582

@Lyrkan
Copy link
Collaborator Author

Lyrkan commented Apr 14, 2020

@piotr-cz Not sure what you mean there, copyFiles(...) never accepted globs in from (unless you're talking about #221 which was not merged).

As explained in #582 you should be able to achieve more or less the same thing using pattern and includeSubdirectories.

@piotr-cz
Copy link

Thanks @Lyrkan

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.

None yet

4 participants