Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/adapter-vercel/image-loader.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/**
* https://vercel.com/docs/concepts/image-optimization
*/
export default function loader(src: string, width: number, options?: { quality?: number }): string;
49 changes: 49 additions & 0 deletions packages/adapter-vercel/image-loader.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// https://vercel.com/docs/concepts/image-optimization

/**
* @param {string} src
Comment on lines +1 to +4

@benmccann benmccann Oct 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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) {

@benmccann benmccann Oct 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const url = new URL(src, 'http://n'); // If the base is a relative URL, we need to add a dummy host to the URL
if (url.pathname === '/_vercel/image') {
set_param(url, 'w', width);
set_param(url, 'q', options?.quality ?? 75, false);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is rather surprising to me. I'd expect if the quality option is not set then the q parameter should not be set either rather than defaulting to 75

} else {
url.pathname = `/_vercel/image`;
Comment thread
benmccann marked this conversation as resolved.
Outdated
set_param(url, 'url', src);
set_param(url, 'w', width);
set_param(url, 'q', options?.quality ?? 75);
}
return src === url.href ? url.href : relative_url(url);
}

/**
* @param {URL} url
*/
function relative_url(url) {
const { pathname, search } = url;
return `${pathname}${search}`;
}
/**
* @param {URL} url
* @param {string} param
* @param {any} value
* @param {boolean} [override]
*/
function set_param(url, param, value, override = true) {
if (value === undefined) {
return;
}

if (value === null) {
if (override || url.searchParams.has(param)) {
url.searchParams.delete(param);
}
} else {
if (override || !url.searchParams.has(param)) {
url.searchParams.set(param, value);
}
}
}
11 changes: 10 additions & 1 deletion packages/adapter-vercel/index.d.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,16 @@
import { Adapter } from '@sveltejs/kit';
import './ambient.js';

export default function plugin(config?: Config): Adapter;
export default function plugin(
config?: Config & {
/**
* Enable or disable Vercel's image optimization. This is enabled by default if you have
* defined the Vercel loader in `kit.images.loader`, else disabled by default.

@benmccann benmccann Oct 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to show how to do this. E.g. do you set kit.images.loader: '@sveltejs/adapter-vercel/image-loader'? There's probably some sort of documentation we should add in documentation/docs/25-build-and-deploy/90-adapter-vercel.md

* https://vercel.com/docs/concepts/image-optimization
*/
images?: boolean;
}
): Adapter;

export interface ServerlessConfig {
/**
Expand Down
14 changes: 14 additions & 0 deletions packages/adapter-vercel/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -407,8 +407,22 @@ function static_vercel_config(builder) {
overrides[page.file] = { path: overrides_path };
}

/** @type {Record<string, any> | undefined} */
let images = undefined;
const img_config = builder.config.kit.images;
if (config.images || img_config.loader === '@sveltejs/adapter-vercel/image-loader') {
images = {
sizes: img_config.sizes,
domains: img_config.domains,
// TODO should we expose the following and some other optional options through the adapter?
formats: ['image/avif', 'image/webp'],
minimumCacheTTL: 300
};
}

return {
version: 3,
images,
routes: [
...prerendered_redirects,
{
Expand Down
4 changes: 4 additions & 0 deletions packages/adapter-vercel/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
"types": "./index.d.ts",
"import": "./index.js"
},
"./image-loader": {
"types": "./image-loader.d.ts",
"import": "./image-loader.js"
},
"./package.json": "./package.json"
},
"types": "index.d.ts",
Expand Down
1 change: 1 addition & 0 deletions packages/kit/scripts/generate-dts.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ createBundle({
'@sveltejs/kit/vite': 'src/exports/vite/index.js',
'$app/environment': 'src/runtime/app/environment.js',
'$app/forms': 'src/runtime/app/forms.js',
'$app/images': 'src/runtime/app/images.js',
'$app/navigation': 'src/runtime/app/navigation.js',
'$app/paths': 'src/runtime/app/paths.js',
'$app/stores': 'src/runtime/app/stores.js'
Expand Down
20 changes: 20 additions & 0 deletions packages/kit/src/core/config/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,12 @@ const options = object(
errorTemplate: string(join('src', 'error.html'))
}),

images: object({
domains: string_array([]),
loader: string(null),
sizes: number_array([640, 828, 1200, 1920, 3840])
}),

inlineStyleThreshold: number(0),

moduleExtensions: string_array(['.js', '.ts']),
Expand Down Expand Up @@ -354,6 +360,20 @@ function string(fallback, allow_empty = true) {
});
}

/**
* @param {number[] | undefined} [fallback]
* @returns {Validator}
*/
function number_array(fallback) {
return validate(fallback, (input, keypath) => {
if (!Array.isArray(input) || input.some((value) => typeof value !== 'number')) {
throw new Error(`${keypath} must be an array of numbers, if specified`);
}

return input;
});
}

/**
* @param {string[] | undefined} [fallback]
* @returns {Validator}
Expand Down
25 changes: 25 additions & 0 deletions packages/kit/src/exports/public.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,31 @@ export interface KitConfig {
*/
errorTemplate?: string;
};
/**
* Image optimization configuration
*/
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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 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.

@benmccann benmccann Oct 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* It optionally also takes third parameter for options.
*
* ```js
* export default function loader(src, width, opts) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move width inside the opts? it seems a bit surprising to me to just have that one single parameter outside the options

* return `https://example.com/${src}?w=${width}&q=${opts.quality || 75}`;
* }
* ```
*/
loader?: string;
/**
* Which srcset sizes to generate

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dummdidumm dummdidumm Oct 6, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@benmccann benmccann Oct 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

* @default [640, 828, 1200, 1920, 3840]
*/
sizes?: number[];
/**
* Which external domains to trust when optimizing images

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dummdidumm dummdidumm Oct 6, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@benmccann benmccann Oct 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

*/
domains?: string[];
};
/**
* Inline CSS inside a `<style>` block at the head of the HTML. This option is a number that specifies the maximum length of a CSS file to be inlined. All CSS files needed for the page and smaller than this value are merged and inlined in a `<style>` block.
*
Expand Down
13 changes: 13 additions & 0 deletions packages/kit/src/exports/vite/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,19 @@ function kit({ svelte_config }) {
}
`;
}

case '\0__sveltekit/images': {
const { images } = svelte_config.kit;
const loader = images.loader
? `export { default as loader } from '${images.loader}';`
: 'export function loader(src) { console.warn("No image loader in kit.config.kit.images.loader set, images will not be optimized."); return src; }';

return dedent`
export const sizes = ${JSON.stringify(images.sizes)};
export const domains = ${JSON.stringify(images.domains)};
${loader}
`;
}
}
}
};
Expand Down
47 changes: 47 additions & 0 deletions packages/kit/src/runtime/app/images.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { DEV } from 'esm-env';
import { sizes, loader, domains } from '__sveltekit/images';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by that? How else would you get the config into the app?

@benmccann benmccann Oct 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* @param {string} src
* @param {any} [options]
* @returns {{ src: string, srcset?: string }}
*/
export function getImage(src, options) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we might want width and height here as required parameters.

It's much nicer to write this:

<img {...getImage('./my-image.jpg', 512, 256)} alt="my image" />

Than this:

<img {...getImage('./my-image.jpg', { width: 512, height: 256})} alt="my image" />

I do like the explicitness of width and height being in the options object, but then you end up with })} which is quite hard to parse and it just sort of seems like a lot of characters to repeat.

if (DEV) {
if (!matches_domain(src)) {
console.warn(
`$app/images: Image src '${src}' does not match any of the allowed domains and will therefore not be optimized.`
);
}
return { srcset: src, src };
}

if (!matches_domain(src)) {
return { src };
}

const srcset = sizes

@benmccann benmccann Oct 9, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand how this is supposed to be used. If the img tag doesn't have the sizes attribute set there's no point in providing a srcset. sizes is very rarely used, so it seems like generating this will mostly be wasted.

Also, I think Vercel is the only CDN that supports sizes. I think we might be better off just having each CDN implement getImage since the logic is generally very simple and it's only Vercel that needs this logic

@benmccann benmccann Oct 12, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back and looked at your old PR as saw that the Image component was setting sizes automatically there: https://github.com/sveltejs/kit/blob/6bca66026e3029f77c1a78b4c67b934a09e7547c/packages/image/src/Image.svelte

I'm not sure if you intended to do that here as well and just missed copying it over or if you meant to do it differently here? I've added support for sizes in the static preprocessor.

sizes is one place where a dynamic preprocessor would be nice. It feels awkward to pass it in via getImage, but a preprocessor could let you write it normally as an attribute and then rewrite it as an arg to getImage. Though usage is rare enough it probably doesn't matter too much if it's slightly more awkward to write.

.map((size) => {
const url = loader(src, size, options);
const w = size + 'w';
return `${url} ${w}`;
})
.join(', ');
const _src = loader(src, sizes[sizes.length - 1], options);

// Order of attributes is important here as they are set in this order
// and having src before srcset would result in a flicker

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return { srcset, src: _src };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect width and height to be returned here. Otherwise you end up needing to specify them both in getImage and the img tag

}

/**
* @param {string} src
*/
function matches_domain(src) {

@benmccann benmccann Oct 9, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this is mostly bloating the runtime as most CDNs don't need this

const url = new URL(src, 'http://n'); // if src is protocol relative, use dummy domain
if (url.href === src) {
return domains.some((domain) => url.hostname === domain);
} else {
return true; // relative urls are always ok
}
}
7 changes: 7 additions & 0 deletions packages/kit/src/types/ambient.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,10 @@ declare module '__sveltekit/paths' {
export function override(paths: { base: string; assets: string }): void;
export function set_assets(path: string): void;
}

/** Internal version of $app/images */
declare module '__sveltekit/images' {
export let sizes: number[];
export let loader: (url: string, size: number, opts?: any) => string;
export let domains: string[];
}