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.

Sign up for GitHub

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

[enhanced-image] support svelte 5 + fix types #12822

Merged
merged 9 commits into from
Nov 1, 2024
Merged

[enhanced-image] support svelte 5 + fix types #12822

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
53f24a9
[enhanced-image] support svelte 5 + fix types
pngwn Oct 16, 2024
081fefe
Apply suggestions from code review
pngwn Oct 16, 2024
8221b82
chore: add ts-ignore to appease ecosystem CI
benmccann Oct 17, 2024
deabf85
remove estree-walker
benmccann Nov 1, 2024
6cad2f7
update svelte 5 to final release
benmccann Nov 1, 2024
de2db7a
fix peer dep range. svelte 4 is no longer supported
benmccann Nov 1, 2024
69dd692
Create witty-years-relate.md
benmccann Nov 1, 2024
bd1f53b
merge main
benmccann Nov 1, 2024
fe5becc
Merge branch 'eh-im' of github.com:sveltejs/kit into eh-im
benmccann Nov 1, 2024
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
7 changes: 4 additions & 3 deletions packages/enhanced-img/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,16 @@
"dependencies": {
"magic-string": "^0.30.5",
"svelte-parse-markup": "^0.1.5",
"vite-imagetools": "^7.0.1"
"vite-imagetools": "^7.0.1",
"zimmerframe": "^1.1.2"
},
"devDependencies": {
"@types/estree": "^1.0.5",
"@types/node": "^18.19.48",
"estree-walker": "^3.0.3",
"rollup": "^4.14.2",
"svelte": "^4.2.10",
"typescript": "^5.3.3",
"svelte": "^5.0.0-next.0",
benmccann marked this conversation as resolved.
Show resolved Hide resolved
"typescript": "^5.6.3",
Copy link
Member Author

Choose a reason for hiding this comment

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

Required to support top-level jsdoc type imports.

"vite": "^5.3.2",
"vitest": "^2.0.1"
},
Expand Down
124 changes: 83 additions & 41 deletions packages/enhanced-img/src/preprocessor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
/** @import { AST } from 'svelte/compiler' */

import { existsSync } from 'node:fs';
import * as path from 'node:path';

import MagicString from 'magic-string';
import { asyncWalk } from 'estree-walker';
import { walk } from 'zimmerframe';
Comment on lines -5 to +7
Copy link
Member Author

Choose a reason for hiding this comment

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

asyncWalk didn't like my types.

import { VERSION } from 'svelte/compiler';
import { parse } from 'svelte-parse-markup';

Expand Down Expand Up @@ -32,7 +34,7 @@ export function image(opts) {
}

const s = new MagicString(content);
const ast = parse(content, { filename });
const ast = parse(content, { filename, modern: true });
Copy link
Member Author

Choose a reason for hiding this comment

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

its 2024


/**
* Import path to import name
Expand All @@ -49,20 +51,26 @@ export function image(opts) {
const consts = new Map();

/**
* @param {import('svelte/types/compiler/interfaces').TemplateNode} node
* @param {{ type: string, start: number, end: number, raw: string }} src_attribute
* @param {import('svelte/compiler').AST.RegularElement} node
* @param {AST.Text | AST.ExpressionTag} src_attribute
Comment on lines +54 to +55
Copy link
Member Author

Choose a reason for hiding this comment

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

Use real types

* @returns {Promise<void>}
*/
async function update_element(node, src_attribute) {
// TODO: this will become ExpressionTag in Svelte 5
if (src_attribute.type === 'MustacheTag') {
const src_var_name = content
.substring(src_attribute.start + 1, src_attribute.end - 1)
.trim();
s.update(node.start, node.end, dynamic_img_to_picture(content, node, src_var_name));
return;
} else if (src_attribute.type === 'AttributeShorthand') {
const src_var_name = content.substring(src_attribute.start, src_attribute.end).trim();
if (src_attribute.type === 'ExpressionTag') {
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed in the new AST.

const start =
'end' in src_attribute.expression
? src_attribute.expression.end
: src_attribute.expression.range?.[0];
const end =
'start' in src_attribute.expression
? src_attribute.expression.start
: src_attribute.expression.range?.[1];

if (typeof start !== 'number' || typeof end !== 'number') {
throw new Error('ExpressionTag has no range');
}
Comment on lines +60 to +71
Copy link
Member Author

Choose a reason for hiding this comment

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

This is technically more correct although I'm unsure we would ever realistically hit this issue. It was required for typescript to shut up tho.

The AST relating to locations did change very slightly with v5s modern AST, using src_attribute.start and src_attribute.end was incorrect.

const src_var_name = content.substring(start, end).trim();

s.update(node.start, node.end, dynamic_img_to_picture(content, node, src_var_name));
return;
}
Expand All @@ -73,10 +81,10 @@ export function image(opts) {
const sizes = get_attr_value(node, 'sizes');
const width = get_attr_value(node, 'width');
url += url.includes('?') ? '&' : '?';
if (sizes) {
if (sizes && 'raw' in sizes) {
url += 'imgSizes=' + encodeURIComponent(sizes.raw) + '&';
}
if (width) {
if (width && 'raw' in width) {
Comment on lines +84 to +87
Copy link
Member Author

Choose a reason for hiding this comment

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

typescript

url += 'imgWidth=' + encodeURIComponent(width.raw) + '&';
}
url += 'enhanced';
Expand Down Expand Up @@ -119,23 +127,35 @@ export function image(opts) {
}
}

/**
* @type {Array<ReturnType<typeof update_element>>}
*/
const pending_ast_updates = [];
Copy link
Member Author

Choose a reason for hiding this comment

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

To store promises.

// TODO: switch to zimmerframe with Svelte 5
// @ts-ignore
await asyncWalk(ast.html, {
/**
* @param {import('svelte/types/compiler/interfaces').TemplateNode} node
*/
async enter(node) {
if (node.type === 'Element') {
// Compare node tag match
if (node.name === 'enhanced:img') {

walk(
/** @type {import('svelte/compiler').AST.Root} */ (ast),
{},
{
_(_, { next }) {
next();
},
/** @param {import('svelte/compiler').AST.RegularElement} node */
// @ts-ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot for the life of me get this to understand me

Copy link
Member

Choose a reason for hiding this comment

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

the trick is for the first argument to walk to be a union of all the node types inside Root (this would be the type argument to walk if you were being explicit, but it's also inferred)

RegularElement(node) {
if ('name' in node && node.name === 'enhanced:img') {
// Compare node tag match
const src = get_attr_value(node, 'src');
if (!src) return;
await update_element(node, src);

if (!src || typeof src === 'boolean') return;

pending_ast_updates.push(update_element(node, src));
Copy link
Member Author

Choose a reason for hiding this comment

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

walk isn't async to we start the work and store the promise for later.

}
}
}
});
);

await Promise.all(pending_ast_updates);
Copy link
Member Author

Choose a reason for hiding this comment

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

We make sure all node modifications have been completed before proceeding.


// add imports and consts to <script module> block
let text = '';
Expand Down Expand Up @@ -215,22 +235,33 @@ export function parseObject(str) {
}

/**
* @param {import('svelte/types/compiler/interfaces').TemplateNode} node
* @param {import('../types/internal.ts').TemplateNode} node
* @param {string} attr
* @returns {AST.Text | AST.ExpressionTag | undefined}
*/
function get_attr_value(node, attr) {
if (!('type' in node) || !('attributes' in node)) return;
const attribute = node.attributes.find(
/** @param {any} v */ (v) => v.type === 'Attribute' && v.name === attr
);

if (!attribute) return;
// console.log(attribute);

pngwn marked this conversation as resolved.
Show resolved Hide resolved
if (!attribute || !('value' in attribute) || typeof attribute.value === 'boolean') return;

return attribute.value[0];
// Check if value is an array and has at least one element
if (Array.isArray(attribute.value)) {
if (attribute.value.length > 0) return attribute.value[0];
return;
}

// If it's not an array or is empty, return the value as is
return attribute.value;
}

/**
* @param {string} content
* @param {Array<import('svelte/types/compiler/interfaces').BaseDirective | import('svelte/types/compiler/interfaces').Attribute | import('svelte/types/compiler/interfaces').SpreadAttribute>} attributes
* @param {import('../types/internal.ts').Attribute[]} attributes
* @param {{
* src: string,
* width: string | number,
Expand All @@ -239,7 +270,7 @@ function get_attr_value(node, attr) {
*/
function serialize_img_attributes(content, attributes, details) {
const attribute_strings = attributes.map((attribute) => {
if (attribute.name === 'src') {
if ('name' in attribute && attribute.name === 'src') {
return `src=${details.src}`;
}
return content.substring(attribute.start, attribute.end);
Expand All @@ -250,8 +281,13 @@ function serialize_img_attributes(content, attributes, details) {
/** @type {number | undefined} */
let user_height;
for (const attribute of attributes) {
if (attribute.name === 'width') user_width = parseInt(attribute.value[0].raw);
if (attribute.name === 'height') user_height = parseInt(attribute.value[0].raw);
if ('name' in attribute && 'value' in attribute) {
const value = Array.isArray(attribute.value) ? attribute.value[0] : attribute.value;
if (typeof value === 'object' && 'raw' in value) {
if (attribute.name === 'width') user_width = parseInt(value.raw);
if (attribute.name === 'height') user_height = parseInt(value.raw);
}
}
}
if (!user_width && !user_height) {
attribute_strings.push(`width=${details.width}`);
Expand Down Expand Up @@ -283,13 +319,15 @@ function stringToNumber(param) {
/**
* @param {Map<string,string>} consts
* @param {string} content
* @param {import('svelte/types/compiler/interfaces').TemplateNode} node
* @param {import('svelte/compiler').AST.RegularElement} node
Copy link
Member Author

Choose a reason for hiding this comment

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

More specific type, we only pass elements to this fn.

* @param {import('vite-imagetools').Picture} image
*/
function img_to_picture(consts, content, node, image) {
/** @type {Array<import('svelte/types/compiler/interfaces').BaseDirective | import('svelte/types/compiler/interfaces').Attribute | import('svelte/types/compiler/interfaces').SpreadAttribute>} attributes */
/** @type {import('../types/internal.ts').Attribute[]} attributes */
const attributes = node.attributes;
const index = attributes.findIndex((attribute) => attribute.name === 'sizes');
const index = attributes.findIndex(
(attribute) => 'name' in attribute && attribute.name === 'sizes'
);
let sizes_string = '';
if (index >= 0) {
sizes_string = ' ' + content.substring(attributes[index].start, attributes[index].end);
Expand Down Expand Up @@ -324,19 +362,21 @@ function to_value(consts, src) {
}
return `{${var_name}}`;
}

return `"${src}"`;
}

/**
* For images like `<img src={manually_imported} />`
* @param {string} content
* @param {import('svelte/types/compiler/interfaces').TemplateNode} node
* @param {import('svelte/compiler').AST.RegularElement} node
Copy link
Member Author

Choose a reason for hiding this comment

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

same as sbove.

* @param {string} src_var_name
*/
function dynamic_img_to_picture(content, node, src_var_name) {
/** @type {Array<import('svelte/types/compiler/interfaces').BaseDirective | import('svelte/types/compiler/interfaces').Attribute | import('svelte/types/compiler/interfaces').SpreadAttribute>} attributes */
const attributes = node.attributes;
const index = attributes.findIndex((attribute) => attribute.name === 'sizes');
const index = attributes.findIndex(
(attribute) => 'name' in attribute && attribute.name === 'sizes'
);
let sizes_string = '';
if (index >= 0) {
sizes_string = ' ' + content.substring(attributes[index].start, attributes[index].end);
Expand All @@ -349,8 +389,10 @@ function dynamic_img_to_picture(content, node, src_var_name) {
height: `{${src_var_name}.img.h}`
};

console.log('DYNAMIC', details, src_var_name);

pngwn marked this conversation as resolved.
Show resolved Hide resolved
return `{#if typeof ${src_var_name} === 'string'}
<img ${serialize_img_attributes(content, node.attributes, details)} />
<img ${serialize_img_attributes(content, attributes, details)} />
{:else}
<picture>
{#each Object.entries(${src_var_name}.sources) as [format, srcset]}
Expand Down
11 changes: 10 additions & 1 deletion packages/enhanced-img/test/Input.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

const src = manual_image1;
const images = [manual_image1, manual_image2];
const get_image = (image_key: number) => images[image_key];

let foo: string = 'bar';
</script>
Expand All @@ -28,7 +29,11 @@
alt="sizes test"
/>

<enhanced:img src="./dev.png" on:click={(foo = 'clicked an image!')} alt="event handler test" />
<enhanced:img
Copy link
Member Author

Choose a reason for hiding this comment

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

update the onclick to be correct and use v5 syntax.

src="./dev.png"
onclick={() => (foo = 'clicked an image!')}
alt="event handler test"
/>

<enhanced:img src="$lib/dev.png" alt="alias test" />

Expand All @@ -42,6 +47,10 @@
<enhanced:img src={image} alt="opt-in test" />
{/each}

{#each images as _, i}
<enhanced:img src={get_image(i)} alt="opt-in test" />
{/each}

Comment on lines +50 to +53
Copy link
Member Author

Choose a reason for hiding this comment

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

extra test to make sure we can handle things that aren't just identifiers.

<picture>
<source src="./dev.avif" />
<source srcset="./dev.avif 500v ./bar.avif 100v" />
Expand Down
18 changes: 16 additions & 2 deletions packages/enhanced-img/test/Output.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<script context="module">
<script module>
import __IMPORTED_ASSET_0__ from "./foo.svg";
Copy link
Member Author

Choose a reason for hiding this comment

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

cheeky

const __DECLARED_ASSET_0__ = "__VITE_ASSET__2AM7_y_a__ 1440w, __VITE_ASSET__2AM7_y_b__ 960w";
const __DECLARED_ASSET_1__ = "__VITE_ASSET__2AM7_y_c__ 1440w, __VITE_ASSET__2AM7_y_d__ 960w";
Expand All @@ -13,6 +13,7 @@

const src = manual_image1;
const images = [manual_image1, manual_image2];
const get_image = (image_key: number) => images[image_key];

let foo: string = 'bar';
</script>
Expand All @@ -33,7 +34,7 @@

<picture><source srcset="/1 1440w, /2 960w" sizes="(min-width: 60rem) 80vw, (min-width: 40rem) 90vw, 100vw" type="image/avif" /><source srcset="/3 1440w, /4 960w" sizes="(min-width: 60rem) 80vw, (min-width: 40rem) 90vw, 100vw" type="image/webp" /><source srcset="5 1440w, /6 960w" sizes="(min-width: 60rem) 80vw, (min-width: 40rem) 90vw, 100vw" type="image/png" /><img src="/7" alt="sizes test" width=1440 height=1440 /></picture>

<picture><source srcset="/1 1440w, /2 960w" type="image/avif" /><source srcset="/3 1440w, /4 960w" type="image/webp" /><source srcset="5 1440w, /6 960w" type="image/png" /><img src="/7" on:click={(foo = 'clicked an image!')} alt="event handler test" width=1440 height=1440 /></picture>
<picture><source srcset="/1 1440w, /2 960w" type="image/avif" /><source srcset="/3 1440w, /4 960w" type="image/webp" /><source srcset="5 1440w, /6 960w" type="image/png" /><img src="/7" onclick={() => (foo = 'clicked an image!')} alt="event handler test" width=1440 height=1440 /></picture>

<picture><source srcset="/1 1440w, /2 960w" type="image/avif" /><source srcset="/3 1440w, /4 960w" type="image/webp" /><source srcset="5 1440w, /6 960w" type="image/png" /><img src="/7" alt="alias test" width=1440 height=1440 /></picture>

Expand Down Expand Up @@ -65,6 +66,19 @@
{/if}
{/each}

{#each images as _, i}
{#if typeof get_image(i) === 'string'}
<img src={get_image(i).img.src} alt="opt-in test" width={get_image(i).img.w} height={get_image(i).img.h} />
{:else}
<picture>
{#each Object.entries(get_image(i).sources) as [format, srcset]}
<source {srcset} type={'image/' + format} />
{/each}
<img src={get_image(i).img.src} alt="opt-in test" width={get_image(i).img.w} height={get_image(i).img.h} />
</picture>
{/if}
{/each}

<picture>
<source src="./dev.avif" />
<source srcset="./dev.avif 500v ./bar.avif 100v" />
Expand Down
34 changes: 34 additions & 0 deletions packages/enhanced-img/types/internal.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import type { AST } from 'svelte/compiler';
Copy link
Member Author

Choose a reason for hiding this comment

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

Just some helpful types for internal use.


type ElementLike =
| AST.Component
| AST.TitleElement
| AST.SlotElement
| AST.RegularElement
| AST.SvelteBody
| AST.SvelteComponent
| AST.SvelteDocument
| AST.SvelteElement
| AST.SvelteFragment
| AST.SvelteHead
| AST.SvelteOptionsRaw
| AST.SvelteSelf
| AST.SvelteWindow;

type Tag = AST.ExpressionTag | AST.HtmlTag | AST.ConstTag | AST.DebugTag | AST.RenderTag;

type Directive =
| AST.AnimateDirective
| AST.BindDirective
| AST.ClassDirective
| AST.LetDirective
| AST.OnDirective
| AST.StyleDirective
| AST.TransitionDirective
| AST.UseDirective;

type Block = AST.EachBlock | AST.IfBlock | AST.AwaitBlock | AST.KeyBlock | AST.SnippetBlock;

export type TemplateNode = AST.Text | Tag | ElementLike | AST.Comment | Block;

export type Attribute = AST.Attribute | AST.SpreadAttribute | Directive;
Loading
Loading