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: caching of generated images #661

Merged
merged 1 commit into from
Mar 28, 2024
Merged

Conversation

pzerelles
Copy link
Contributor

@pzerelles pzerelles commented Nov 29, 2023

Closes #522

  • Quick Checklist
  • I have read the contributing guidelines
  • I have written new tests, as applicable (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • I have added a changeset, if applicable
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Feature

  • What is the new behavior (if this is a feature change)?
    Generated images can be cached to skip transformation of unchanged images.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this
    PR?)
    No

  • Other information:

Copy link

changeset-bot bot commented Nov 29, 2023

🦋 Changeset detected

Latest commit: b0b152f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
vite-imagetools Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pzerelles pzerelles changed the title Cacheing of generated images Caching of generated images Nov 29, 2023
Copy link

codecov bot commented Nov 29, 2023

Codecov Report

Attention: Patch coverage is 83.33333% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 95.47%. Comparing base (dc2f16f) to head (b0b152f).

Files Patch % Lines
packages/vite/src/index.ts 84.37% 10 Missing ⚠️
packages/vite/src/utils.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #661      +/-   ##
==========================================
+ Coverage   94.17%   95.47%   +1.30%     
==========================================
  Files          33       33              
  Lines        1235     1283      +48     
  Branches      206      224      +18     
==========================================
+ Hits         1163     1225      +62     
+ Misses         72       58      -14     
Flag Coverage Δ
imagetools-core 95.47% <83.33%> (+1.30%) ⬆️
vite-imagetools 95.47% <83.33%> (+1.30%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@pzerelles pzerelles changed the title Caching of generated images feat: caching of generated images Nov 29, 2023
packages/vite/src/index.ts Outdated Show resolved Hide resolved
@AdamMerrifield
Copy link
Contributor

Are there any other issues with this other than the conflicts that need to be resolved? I have tested it locally and it is working well for me.

@benmccann
Copy link
Collaborator

I was hoping to get caching support in Vite first, but I don't think that's going to happen soon enough, so I plan to get this PR in. I won't be able to look at it for the next week, but hopefully after that.

@benmccann
Copy link
Collaborator

@pzerelles heads up that this PR needs a rebase

@Aragur
Copy link

Aragur commented Mar 9, 2024

Awesome PR, I was looking for a way to cache my pictures. Pipeline takes 30 minutes because I have many images.
Are there any updates on this PR, when will it be merged? Is there anything I can do to help?

@benmccann
Copy link
Collaborator

This one has a merge conflict I think from merging your other PR. Can you update it? Thanks!

docs/guide/caching.md Outdated Show resolved Hide resolved
@julien-blanchon
Copy link

Wow I would love to see this merged !

@julien-blanchon
Copy link

Is this stable enouth to be use in dev ? I have almost 20min of build time on Vercel (almost 40 image to be optimize by imagetools) ... this is a nightmare for development. I'm really thinking to stop using imagetools because of that ?

@pzerelles
Copy link
Contributor Author

Is this stable enouth to be use in dev ? I have almost 20min of build time on Vercel (almost 40 image to be optimize by imagetools) ... this is a nightmare for development. I'm really thinking to stop using imagetools because of that ?

I've been using it in dev and production for half a year already.

@julien-blanchon
Copy link

Is this stable enouth to be use in dev ? I have almost 20min of build time on Vercel (almost 40 image to be optimize by imagetools) ... this is a nightmare for development. I'm really thinking to stop using imagetools because of that ?

I've been using it in dev and production for half a year already.

How did you specify pnpm/npm/yarn to fetch this branch ?
Any suggestion for Vercel, I suppose it work out the box

@pzerelles
Copy link
Contributor Author

Is this stable enouth to be use in dev ? I have almost 20min of build time on Vercel (almost 40 image to be optimize by imagetools) ... this is a nightmare for development. I'm really thinking to stop using imagetools because of that ?

I've been using it in dev and production for half a year already.

How did you specify pnpm/npm/yarn to fetch this branch ?

Any suggestion for Vercel, I suppose it work out the box

I use a personal build created from my forked repository. But I hope this will be merged really soon. We are working on it.

packages/vite/src/utils.ts Outdated Show resolved Hide resolved
packages/vite/src/utils.ts Outdated Show resolved Hide resolved
packages/vite/src/index.ts Outdated Show resolved Hide resolved
docs/guide/caching.md Outdated Show resolved Hide resolved
packages/vite/src/__tests__/main.test.ts Outdated Show resolved Hide resolved
packages/vite/src/index.ts Outdated Show resolved Hide resolved
@benmccann
Copy link
Collaborator

It looks to me like if a user changes the way in which they want an image to be transformed that those changes won't get picked up because we prefer the cache.

I think we may be able to remove the need for an index.json file if we write a copy of the image file to the cache directory where the filename is the ID returned by generateImageID. Then we could almost use the logic that exists today, but instead of checking the in-memory generatedImages cache we could check the cache directory.

@pzerelles
Copy link
Contributor Author

pzerelles commented Mar 27, 2024

It looks to me like if a user changes the way in which they want an image to be transformed that those changes won't get picked up because we prefer the cache.

I think we may be able to remove the need for an index.json file if we write a copy of the image file to the cache directory where the filename is the ID returned by generateImageID. Then we could almost use the logic that exists today, but instead of checking the in-memory generatedImages cache we could check the cache directory.

I see your point with the user changes. I need to include the config somewhere...

The main purpose of the index.json file is to provide the metadata without reading the source image, which could be quite large. generateImageID uses the srcURL, config and imageBuffer to generate the cache, but to recreate it we would need to ready the source image, too.

@pzerelles
Copy link
Contributor Author

I refactored the whole PR to use the ID from generateImageID. Since user changes will also change that ID, the former problem is solved. I adjusted tests where necessary.

@AdamMerrifield
Copy link
Contributor

@pzerelles while using this, i have found that it is possible for 0 byte files to be stored in the cache directory if a build is cancelled while generating images.

I sent a PR to resolve this issue pzerelles#1

docs/guide/caching.md Outdated Show resolved Hide resolved
docs/guide/caching.md Outdated Show resolved Hide resolved
@benmccann benmccann merged commit 4819fc1 into JonasKruckenberg:main Mar 28, 2024
5 of 6 checks passed
@julien-blanchon
Copy link

Yayyy, will this be release soon ? How can I update in the meantime ?

@benmccann
Copy link
Collaborator

Yes, it will be released before too long. It will be available when #707 is merged. You can subscribe to that PR for updates

@julien-blanchon
Copy link

julien-blanchon commented Mar 29, 2024

Humm I don't know why but it's seems to not work with 7.0, and

imagetools({
			cache: {
				enabled: true,
				dir: './node_modules/.cache/imagetools',
			}
		}),

In the vite.config.ts.

I'm using Sveltekit with a bunch of `?enhanced' imports.

The node_modules/.cache/imagetools is create but remains empty

@benmccann
Copy link
Collaborator

You'll need to wait for sveltejs/kit#12055 for @sveltejs/enhanced-img. You shouldn't need to setup image tools.in your Vite config with @sveltejs/enhanced-img

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.

Cache generated images?
6 participants