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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 2 commits into from

Conversation

pngwn
Copy link
Member

@pngwn pngwn commented Oct 16, 2024

This PR updates enhanced-image to support svelte 5, with updated tests and types.

It was a little more involved than I anticipated, mostly due to type issues altho there were some changes in the AST from what I can see when witching to modern mode.

This PR includes the following changes:

  • Update devDeps to svelte 5 (currently next but we can change when we release)
  • Use the new public AST types instead of the old private ones
  • asyncWalker from estree-walker didn't like the new tpes so i switched to zimmerframe.
  • zimmerframe's walk is not asynchronous, so I had to modify the logic slightly to wait for the AST updates to be performed (as the updates are asynchronous).
  • Updated the expression test to look for ExpressionTag instead of MustacheTag.
  • Some general hardening, mainly to satisfy typescript but it might be more resilient too.
  • A little type narrowing where appropriate.
  • Added some internal compound types to preserve our sanity.
  • Updated the tests to match the v5 output, I also changed the on:click to onclick and corrected the syntax.
  • Added an extra test to satisfy my nervousness.
  • Updated typescript to support top level jsdoc type imports.

I will comment the code directly to clarify these changes.

Closes #12686.


Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

Copy link

changeset-bot bot commented Oct 16, 2024

⚠️ No Changeset found

Latest commit: 081fefe

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link
Member Author

@pngwn pngwn left a comment

Choose a reason for hiding this comment

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

Commented stuff for clarity

"svelte": "^4.2.10",
"typescript": "^5.3.3",
"svelte": "^5.0.0-next.0",
"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.

Comment on lines -5 to +7
import { asyncWalk } from 'estree-walker';
import { walk } from 'zimmerframe';
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.

@@ -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

Comment on lines +54 to +55
* @param {import('svelte/compiler').AST.RegularElement} node
* @param {AST.Text | AST.ExpressionTag} src_attribute
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

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.

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.

@@ -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.

Comment on lines +50 to +53
{#each images as _, i}
<enhanced:img src={get_image(i)} alt="opt-in test" />
{/each}

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.

@@ -1,4 +1,4 @@
<script context="module">
<script module>
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

@@ -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.

Co-authored-by: Rich Harris <richard.a.harris@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Svelte 5 ecosystem CI is failing
3 participants