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

Manifest key problem with "build cache" + copyFiles with non-fresh cache #936

Closed
weaverryan opened this issue Feb 19, 2021 · 6 comments
Closed

Comments

@weaverryan
Copy link
Member

Hi!

I just hit a bug when you combine Encore.enableBuildCache with Encore.copyFiles(). When you build fresh (no cache, easy to replicate by removing node_modules/.cache/), the manifest.json contains entries like this:

"build/images/chart.png": "/build/images/chart.db1f878a.png",

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:

"build/images/chart.db1f878a.png": "/build/images/chart.db1f878a.png",

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 that copyFiles() is the only place in the system that uses file-loader... which is probably related.

Cheers!

@weaverryan
Copy link
Member Author

I was correct about this being a bug with cache + webpack-manifest-plugin, and I can see why - the file-loader is never called... and that library relies on a hook in file-loader to collect the filenames.

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.

@kl3sk
Copy link

kl3sk commented Mar 4, 2021

FYI I ran into the problem, disabling enableBuildCache solved my problems.

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

@bobvandevijver
Copy link
Contributor

@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)?

@weaverryan
Copy link
Member Author

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.

@bobvandevijver
Copy link
Contributor

bobvandevijver commented May 17, 2021

@weaverryan Looks like it is the exact same issue as described in webdeveric/webpack-assets-manifest@0b323fc. The loaderContext.emitFile isn't called when Webpack is run with a valid cache: the whole file-loader seems to be skipped...

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 asset.info is empty when the assets stats are retrieved.

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!

bobvandevijver added a commit to bobvandevijver/webpack-encore that referenced this issue May 18, 2021
bobvandevijver added a commit to bobvandevijver/webpack-encore that referenced this issue May 18, 2021
bobvandevijver added a commit to bobvandevijver/webpack-encore that referenced this issue May 18, 2021
bobvandevijver added a commit to bobvandevijver/webpack-encore that referenced this issue May 18, 2021
weaverryan pushed a commit to bobvandevijver/webpack-encore that referenced this issue May 31, 2021
weaverryan added a commit that referenced this issue May 31, 2021
…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
@bobvandevijver
Copy link
Contributor

This can be closed 🎉

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

No branches or pull requests

3 participants