feat: add dynamic image optimization #10323
Conversation
part of #241 closes #9787 This adds image optimization through a new $app/images import. It's deliberately low level: The only export is getImage which you pass an image src and it returns an object containing src and srcset (possibly more?) values which you spread on an img tag. In order to use this you need to define a path to a loader in kit.config.images. The loader takes the original img src and a width and returns a URL pointing to the optimized image. You can also modify the number of sizes and trusted domains.
|
dummdidumm
force-pushed
the
image-optimization
branch
from
43098b2
to
3bf5d7f
Compare
|
LGTM, singular makes sense. Even though it is multiple images they are representing the same image resource. The limited scope is a lot easier to integrate into existing components. |
There was a problem hiding this comment.
I'm not sure I like the idea of telling people to use this for images that are in your repository without offering a preprocessor. It's much harder for users to setup as they must update every img tag on their site and it also makes it very hard to switch between static and dynamic providers
This comment was marked as off-topic.
This comment was marked as off-topic.
Yes, that's deliberate - the insight in #9787 was that they are very different things and therefore should be treated differently. Trying to shoehorn both into one would result in suboptimal solutions for both. This way, each use case can be optimized for. You can also abstract both away into one image component that fits your needs. |
|
Perhaps under the covers. But I feel the user interface shouldn't involve them having to write out a function call for every image. We should still pair it with a preprocessor that takes care of it |
|
Assuming we create another preprocessor for static image optimization, how would we then be able to tell when to use which preprocessor? How would the user know which one is used? |
|
You could set a default via an option in the preprocessor or you could make them separate preprocessors. I doubt you'd want to mix and match. I think you'd just choose a provider and go with them. If there really is a need though we could offer comments to change the behavior like I think the API here would make sense for things like rendering an image the user has uploaded where the details are saved in a database or one-off usage where you've just got a handful of images you want to handle differently. For images that are included in your project's repository, you probably want the preprocessor which could be backed by either the build-time implementation or a CDN implementation like Vercel. |
| // https://vercel.com/docs/concepts/image-optimization | ||
|
|
||
| /** | ||
| * @param {string} src |
There was a problem hiding this comment.
| // https://vercel.com/docs/concepts/image-optimization | |
| /** | |
| * @param {string} src | |
| /** | |
| * https://vercel.com/docs/concepts/image-optimization | |
| * @param {string} src |
| * @param {number} width | ||
| * @param {{ quality?: number }} [options] | ||
| */ | ||
| export default function loader(src, width, options) { |
There was a problem hiding this comment.
it'd probably make sense to either generate the types with dts-buddy or use the type defined in the .d.ts file to keep them from going out of sync with each other
packages/kit/src/exports/public.d.ts
Outdated
| */ | ||
| sizes?: number[]; | ||
| /** | ||
| * Which external domains to trust when optimizing images |
There was a problem hiding this comment.
what's the usecase for this? it sort of seems like an oddly-specific feature. I imagine the user could create their own wrapper around getImage if this was something they needed
There was a problem hiding this comment.
It's needed if you want to optimize other images than ones that reside on your domain - else Vercel will not optimize them: https://vercel.com/docs/build-output-api/v3/configuration#images . AFAIK other provides have some variant of this, too, so I added it the general config.
There was a problem hiding this comment.
Ah, makes sense. But could we move these options to the Vercel loader since they're somewhat Vercel-specific? It's a bit confusing to me that some options are here and some are in the adapter, so you need to check two places to see what the available options are. Maybe we should just put them all in the adapter?
| */ | ||
| loader?: string; | ||
| /** | ||
| * Which srcset sizes to generate |
There was a problem hiding this comment.
is this description correct? I thought it was the screen sizes. I know you had a device_sizes variable in #10323 and these look like pretty large numbers. I think only some of these will get generated based on the device being used, which would be helpful to include in the documentation
There was a problem hiding this comment.
You have to tell Vercel (and some other image optimization providers, too) which image widths / sizes / call-it-whatever-you-want are allowed to be generated. 3840 may sound like a big number, but it really isn't when you think about a 4k resolution laptop screen.
There was a problem hiding this comment.
The main problem with this config and the domain config is that image optimization providers may have specific settings / requirements. The question now is - how do you make it so that you don't have to duplicate the config? Either you put a common denominator inside the Kit config, and adapters can read from that config, or you provide that config through the adapters, but then we need a way to get the config from the adapter into Kit.
There was a problem hiding this comment.
I'd not have any common config. E.g. the domains config seems rather Vercel-specific. As an example, I just checked Cloudinary and they seem not to have such an option.
sizes is very Vercel-specific as well. E.g. I just checked Bunny, Cloudinary, and Contentful and it doesn't seem any have such a concept
- https://support.bunny.net/hc/en-us/articles/360027448392-Bunny-Optimizer-Engine-Documentation
- https://github.com/cloudinary/cloudinary_npm/blob/master/types/index.d.ts
- https://www.contentful.com/developers/docs/references/images-api/#/reference
I don't think it's bad if you end up having the same config in different implementations. And in fact it might be helpful because different implementations might have config with the same name that behaves slightly differently
| @@ -0,0 +1,47 @@ | |||
| import { DEV } from 'esm-env'; | |||
| import { sizes, loader, domains } from '__sveltekit/images'; | |||
There was a problem hiding this comment.
I'd prefer not to tie it to SvelteKit, so I think it'd be nicer to create a singleton instance rather than using a virtual module.
There was a problem hiding this comment.
What do you mean by that? How else would you get the config into the app?
There was a problem hiding this comment.
I don't think the app needs it. If you move domains and sizes into adapter-vercel and just use them there then SvelteKit doesn't need the config. adapter-vercel could be given the sizes directly as an adapter option and then it could use it to generate the srcset
packages/kit/src/exports/public.d.ts
Outdated
| */ | ||
| images?: { | ||
| /** | ||
| * Path to a a file that contains a loader that will be used to generate the an image URL out of the given source and width. |
There was a problem hiding this comment.
| * Path to a a file that contains a loader that will be used to generate the an image URL out of the given source and width. | |
| * Path to a file that contains a loader that will be used to generate the an image URL out of the given source and width. |
There was a problem hiding this comment.
Perhaps we should say "module" rather than "file"? It could be good to mention that it would often be a module from an npm package being used here
eltigerchino
added
feature request
|
Will it preprocess CSS as well so to inject f.ex. Perhaps you would just suggest not using images in CSS like in that article suggesting using Would it be interesting to bake in some of those other suggestions to use This is what I came up with for images in the stylesheet: From what I'm getting this will operate more like a preprocessing step and not one that will require the originally proposed |
| } | ||
|
|
||
| // Order of attributes is important here as they are set in this order | ||
| // and having src before srcset would result in a flicker |
There was a problem hiding this comment.
Better safe than sorry, but I haven't noticed any flicker when src comes before srcset in an img tag. Just curious if you have any proof of this? Haven't seen it mentioned anywhere else.
part of #241
closes #9787
This adds image optimization through a new
$app/imagesimport. It's deliberately low level: The only export isgetImagewhich you pass an image src and it returns an object containingsrcandsrcset(possibly more?) values which you spread on an img tag.Example:
In order to use this you need to define a path to a loader in
kit.config.images. The loader takes the original img src and a width and returns a URL pointing to the optimized image. You can also modify the number of sizes and trusted domains.Open questions:
$app/imageandkit.imageor$app/imagesandkit.images?srcsetandsrcshouldgetImagereturn?getSrcset? (probably not; you can create that yourself fromgetImage)Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.