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

feat(cacheDir): add caching independent of webpack's caching #466

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

OlenDavis
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Without this feature, all users of this plugin are strictly dependent on webpack's caching behaviors, which are actually typically very duplicative especially for an operation like image optimization and conversions like this plugin affords. For instance, with Next.js or Nuxt.js you can use webpack's cache.cacheDirectory with cache.type === 'filesystem', and it will create multiple separate webpack caches, leading to at least two independent caches (and therefore redundant transformations) of all the images this plugin outputs. Worse yet, different ways of this plugin operating (a minimizer vs a generator) that perform actually the same underlying transformation will -- especially with the way the plugin currently uses Webpack's caching based on a serialization of the whole minimizer or generator -- cause the transformation to occur and then be cached many more times than necessary.

This PR therefore introduces an option cacheDir. It can be configured for the whole plugin, or a particular minimizer/generator, or even just a specific sharp transformation/preset. It operates at a relatively low level to cache the specific Sharp transformations that are needed directly to the filesystem where configured. Thanks to this, also included is a new option bypassWebpackCache to not unnecessarily rely on the Webpack cache explicitly as a part of this plugin.

Breaking Changes

None that I'm aware of; all tests still pass. And some of the tests have been modified to make use of the cacheDir and bypassWebpackCache options; but additional tests to specifically confirm the new behaviors are not included in this.

Additional Info

Note: This is only integrated into the underlying Sharp transformer.

Reference: You can compare this to the old imagemin-webpack-plugin's cacheFolder behavior. But please note that this behavior is much more sophisticated in two ways:

  • it goes to considerable lengths to ensure thread-safety in even the context of multiple webpack processes running on the same volume referencing the same cacheDir (i.e. for Next.js or Nuxt.js w/ server and browser webpack processes running concurrently).
  • it does (unlike the imagemin-webpack-plugin's cacheFolder) ensure that the cache is unique to both the transformation being done and the original image being transformed. The cache within the cacheDiris organized as follows:{cacheDir}/{hash of the transformation options}/{hash of the original image}`

This PR was branched off of #464 and therefore includes all those changes as well.

Copy link

codecov bot commented Jan 26, 2025

Codecov Report

Attention: Patch coverage is 91.51515% with 14 lines in your changes missing coverage. Please review.

Project coverage is 79.56%. Comparing base (3a924e4) to head (a31f48f).
Report is 25 commits behind head on master.

Files with missing lines Patch % Lines
src/utils.js 87.75% 6 Missing ⚠️
src/cache.js 95.08% 3 Missing ⚠️
src/loader.js 87.50% 3 Missing ⚠️
src/index.js 93.54% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #466      +/-   ##
==========================================
+ Coverage   77.16%   79.56%   +2.39%     
==========================================
  Files           4        5       +1     
  Lines         727      866     +139     
  Branches      282      328      +46     
==========================================
+ Hits          561      689     +128     
- Misses        137      148      +11     
  Partials       29       29              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexander-akait
Copy link
Member

will create multiple separate webpack caches, leading to at least two independent caches (and therefore redundant transformations) of all the images this plugin outputs

Do you use filesystem cache for dev and prod builds?

None that I'm aware of; all tests still pass. And some of the tests have been modified to make use of the cacheDir and bypassWebpackCache options; but additional tests to specifically confirm the new behaviors are not included in this.

I think cacheDir is enough, we can just improve our readme and say what using cacheDir disabling built-in webpack cache

But to be honest, I would have done it differently.

Why? We solve the problem only for our plugin but this problem can occur not only here.

And therefore it would be logical to solve it at the level of webpack itself.

And I see a very simple solution.

We have https://webpack.js.org/configuration/cache/#cachename

Each plugin has unique name for own caches (https://github.com/webpack-contrib/image-minimizer-webpack-plugin/blob/master/src/index.js#L250).

So we can implement (just pseudo code):

cacheName: function (name, defaultCacheDirectory, defaultCacheName) {
  if (/image/i.test(name)) { 
    return path.resolve(defaultCacheDirectory, 'my-image-cache');
  }
  
  return path.resolve(defaultCacheDirectory, defaultCacheName);
}

So it will be possible have multiple different cache for one build.

And thus we do not have to duplicate the code, maintain several logics, we can still use other cache parameters.

What do you think?

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.

2 participants