-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
Manifest key problem with "build cache" + copyFiles with non-fresh cache #936
Comments
I was correct about this being a bug with cache + webpack-manifest-plugin, and I can see why - the Interestingly, https://github.com/webdeveric/webpack-assets-manifest does not have this problem. They DO have a different bug related to cache, however - webdeveric/webpack-assets-manifest#131 Anyways, I'll look there for inspiration to see what hooks they're using for file-loader. |
FYI I ran into the problem, disabling This is how it was configured: .enableBuildCache({
// object of "buildDependencies"
// https://webpack.js.org/configuration/other-options/#cachebuilddependencies
// __filename means that changes to webpack.config.js should invalidate the cache
config: [__filename],
}) |
@weaverryan I just ran into this issue as well... Did you by any chance find a way to solve this (or another place to file an issue)? |
I haven’t had time to look into it yet - I only know that the https://github.com/webdeveric/webpack-assets-manifest library somehow doesn’t have this problem. But diving into these complex kind to understand what they’re doing differently is... quite tricky. |
@weaverryan Looks like it is the exact same issue as described in webdeveric/webpack-assets-manifest@0b323fc. The Looks like the file-loader is deprecated anyways for Webpack v5, so that might be why it doesn't work as expected... The solution the webpack-assets-manifest uses (iterate over the assets to determine their origin) doesn't work at this time for Encore, because the edit: I might have found a solution for the missing info object, so we can use the same fix 😄 Will look at creating a PR as soon as possible! |
…hen building with cache
…devijver) This PR was squashed before being merged into the main branch. Discussion ---------- #936: Fix manifest key problem when using copy files So, the solution for #936 was not complex at all, but creating a failing test was. I needed to update the test scripts in the `package.json`, as webpack simply doesn't write it's cache until the mocha process ends. I did not find an alternative way to trigger the cache files to be written. So, I separated the persistent cache tests into a dedicated directory/file, which is ignored during the normal test run. I added commands to run the persistent cache test both with and without previous cache (using the `CLEAR_PERSISTENT_CACHE` environment variable), and updated the original test command to run the normal test once, and the persistent cache tests twice (so the second run is with a valid cache). I also needed to update the temp directory generation for the persistent cache, as it need to be deterministic over multiple cache runs. This is done with an extra parameter when creating the webpack configuration. This also makes sure that the webpack persistent cache is not shared over multiple tests (as long as you use unique keys obviously. Just let me know if something needs to be done different! Commits ------- c18ac58 #936: Fix manifest key problem when using copy files
This can be closed 🎉 |
Hi!
I just hit a bug when you combine
Encore.enableBuildCache
withEncore.copyFiles()
. When you build fresh (no cache, easy to replicate by removingnode_modules/.cache/
), themanifest.json
contains entries like this:But if you build again when there is already cache available (easy to replicate by running
yarn watch
and modifying any file), then you get:Notice they key has the version.
Btw, my guess is that this is a bug with webpack-manifest-plugin & the build cache, not actually with
copyFiles()
. It's just thatcopyFiles()
is the only place in the system that usesfile-loader
... which is probably related.Cheers!
The text was updated successfully, but these errors were encountered: