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: image preprocessor #10788

Merged
merged 117 commits into from
Nov 11, 2023
Merged

feat: image preprocessor #10788

merged 117 commits into from
Nov 11, 2023

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Sep 26, 2023

Implements the preprocessor described in #241. Please read both the description of that issue and the README in this PR for more details.

Together with with the dynamic solution in #10323 this is most of what we need to handle images. This pair of PRs would replace #9787.

Package setup

I named it @sveltejs/enhanced-img, which is updated from @sveltejs/image in the earlier PR since this one is focused on the static case and we'll have a dynamic one as well

This makes vite-imagetools a dependency. vite-imagetools includes sharp, which is a bit heavy and platform-specific, so we wouldn't want to include it for the dynamic case. However, for the static case, this means users don't need to add two packages to their package.json as in the earlier PR, which required installing vite-imagetools as well.

However, if we have a dynamic preprocessor as well it will have to live in a separate package. This means that users could end up having to install @sveltejs/enhanced-img and @sveltejs/dynamic-img and instantiate both of them. I.e. one possible idea would be to combine this PR with the getImage PR, to have an option to transform

<img src={myImg} width="512" height="256" sizes="(min-width: 960px) 720px, 100vw" alt="my image" />

to:

<script>
  import { getImage } from '@sveltejs/dynamic-img';
</script>

<img {...getImage(myImg, 512, 256, { sizes: '(min-width: 960px) 720px, 100vw' })} alt="my image" />

This could be a little more natural for the user to write.

Performance

Inserting picture markup for each individual image could bloat the HTML-creation JS. E.g. this REPL outputs 42 lines of code in Svelte 5 today while this REPL outputs 67 lines of code. Compare this to a component-based approach where you have 19+89 lines of output for a single image and 22+89 lines of code for two images.

However, we resolve the import to the image and inline the values such that a single image would output something like this and two images would output something like this.

It should be noted that the preprocessor also has the advantage of not needing to ship getWidths to the client, which saves about another 25 lines of code.

There's still some optimization that can be done in the Svelte 5 compiler to optimize the dynamic part a bit to better handle non-reactive values in template strings.

Passing transformed image imports to non-transformed <img>

Right now this only has vite-imagetools transform images with the enhanced-img query parameter to avoid taking over existing imports. We have the preprocessor add this automatically. You must add it manually when you want to dynamically choose an image, but that's probably infrequent enough that it's okay.

The other way of dealing with this would be to transform all imported images by default and users can opt-out with ?url. I think it's pretty rare that you'd want to import an image and not transform it and can't really think of when you'd want to do that, so that option does have an appeal, but it could be unexpected. If we went this route it would be more important to work on our error messages as noted below.

It's an error if you pass a transformed img to a standard img tag. The error message right now isn't great and can be slightly hard to diagnose. It ends up generating HTML that looks like <img src="[object Object]" /> which then makes a request giving the error message:

Error: Not found: /[object%20Object]
    at resolve (packages/kit/src/runtime/server/respond.js:483:13)
    at resolve (packages/kit/src/runtime/server/respond.js:277:5)
    at Object.handle (sites/kit.svelte.dev/src/hooks.server.js:19:9)
    at Module.respond (packages/kit/src/runtime/server/respond.js:274:40)

I can think of a few ways we might be able to detect this:

  • have the Svelte runtime check when setting an img src attribute in DEV mode that it's being passed a string
  • opt all img tags with a mustache src into preprocessing in DEV mode and then add a check that it's being passed a string
  • check for /[object%20Object] as URL in our 404 handling and suggest you might have forgotten a <!-- static-img-enable --> and should view-source: to see where needs to be updated
  • run a check after SSR for src="[object Object]" in the generated page source and print a warning similar to the one above

If there's someplace a user was using an imported image other than an image tag most of these checks wouldn't save us, but that seems less likely to happen. The only thing I can think is if they were trying to output some sort of meta data via {@html}.

Configuring

For now, the preprocessor doesn't take any options. This keeps the API smaller and make it easier to land this PR. We'll probably have to add some options in the future, but it's not necessary to get an initial usable version out.

There are some options like deviceSizes that we may want to share with the dynamic implementation and have live in the Svelte config. I think the dynamic approach needs to advance a bit more before we can say what it should look like though

We could have the user add another instance of vite-imagetools for anything custom they want to do. However, this could be very frustrating because you'd be forced to create an Image component to use with it, which would be not only cumbersome, but would reintroduce the problems we're trying to avoid here by having a preprocessor rather than a component (i.e. event handling and usage of :global for styling). It might also force you to setup svelte-preprocess-import-assets yourself. Exposing defaultDirectives in some manner would probably be quite appreciated

For reference, here are all the options in vite-imagetools
  • include/exclude - it might be better for us to have our own filter or we might want to apply these filters to the URLs we find in img tags to decide whether to process that tag
  • defaultDirectives - could be useful. we would want to ensure correct as directive is used (as=picture unless there's a single format). there aren't too many directives you'd want to set on every image (background, flatten, grayscale, lossless might be some of the most likely). but some are useful - e.g. background plus flatten would further reduce file size. you may also want to create your own presets
  • extendTransforms - a common use case is the low quality placeholder discussed in the next bullet. could also be useful in other advanced cases, but seems uncommon
  • extendOutputFormats - it seems the main use case for this is the low-quality placeholder. i'm not sure this would be possible for users to do with this package without us explicitly adding support for it
  • resolveConfigs - this is pretty niche and not too likely to be used
  • removeMetadata - leaving metadata on optimized could be useful, but I expect uncommon. I think this should be made a directive instead of an option in vite-imagetools

Including in the default SvelteKit template

For now, this is not included in the default template. However, after we've released the dynamic one and let this bake for a little while, I think it would be nice to add it.

Todo

  • Fix TODOs regarding type imports - help would be much appreciated!

Later

@changeset-bot
Copy link

changeset-bot bot commented Sep 26, 2023

🦋 Changeset detected

Latest commit: a56c459

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

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

@nosovk
Copy link
Contributor

nosovk commented Nov 7, 2023

Yesterday Angular rolled a big update, with that functionality included: https://angular.dev/guide/image-optimization
some ideas seem to be useful.

- move ambient declarations into its own file and make sure they are loaded when anything from the package is loaded by importing it in the root types file
- move element augmentation into root types file. Needs to be there because it can't be in an ambient module, the augmentations wouldn't work in such a module
- add fix to SvelteKit to ensure vite.config.js is also loaded, so that when someone uses svelte:image with a vite.config.js/ts its types are loaded
@jasongitmail
Copy link

Great work on this!

@benmccann @Rich-Harris I'm slightly late on this, but a recent thought on naming:

If this were named enhance:img (present tense), it'd mirror use:enhance that exists for forms and remove the need for devs to recall whether past tense or present tense should be used in one situation or the other.

@ecstrema
Copy link
Contributor

ecstrema commented Jul 3, 2024

If this were named enhance:img (present tense), it'd mirror use:enhance that exists for forms and remove the need for devs to recall whether past tense or present tense should be used in one situation or the other.

That's true there. I've stumbled across it a few times, and it got me to the point where I want to use enhance:form. Some sort of standard would be great here. Especially since I believe there will be other enhancements.

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.

7 participants