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: npx svelte-migrate self-closing-tags #12102

Closed
wants to merge 14 commits into from

Conversation

Rich-Harris
Copy link
Member

Companion to sveltejs/svelte#11114. This adds an npx svelte-migrate self-closing-tags migration that replaces all the self-closing non-void elements in your .svelte files.


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 Apr 10, 2024

🦋 Changeset detected

Latest commit: 61c07c2

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

This PR includes changesets to release 1 package
Name Type
svelte-migrate Minor

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

@Rich-Harris
Copy link
Member Author

The lint failure here is caused by a battle between this and Prettier, which turns <slot></slot> back into <slot />. I'm not sure why that's happening — prettier-plugin-svelte is supposed to ignore closing tag style as of version 3 — but we'll need to get to the bottom of it before we can release this.

@jrmajor
Copy link
Contributor

jrmajor commented Apr 11, 2024

@Rich-Harris
Copy link
Member Author

Fantastic, thank you! Updated the migration task to prompt people to upgrade — as soon as it's released, we'll bump the prettier-plugin-svelte version used in this repo and release this

@Conduitry
Copy link
Member

Conduitry commented Apr 12, 2024

We should ideally figure out why we now have simultaneous two versions of esbuild as of this PR.

edit: And two versions of Rollup.

edit: And two versions of Svelte.

@Rich-Harris
Copy link
Member Author

i have absolutely no idea

@Rich-Harris
Copy link
Member Author

downgrading prettier-plugin-svelte back to the previous version doesn't seem to change anything

@Conduitry
Copy link
Member

The two versions of esbuild, at least, appears to be because we upgraded Vite in this PR (probably unnecessarily), which depends on a newer version of esbuild - but other things, like some adapters, still depend on the old version.

@Conduitry
Copy link
Member

I've taken the liberty of pushing 3148f19 - let's see how this looks now.

@Conduitry
Copy link
Member

I think that looks good? For posterity, what I did was grab an old copy of pnpm-lock.yaml from main, run pnpm install again, and commit that.

@Rich-Harris
Copy link
Member Author

yeah, aside from the random failing test — investigating

@Rich-Harris
Copy link
Member Author

The failing test relates to this file:

<svelte:head>
<script type="application/ld+json">
{
JSON.stringify(jsonLd);
}
</script>
</svelte:head>

Looks like sveltejs/prettier-plugin-svelte#424 introduced a change to how JSON+LD scripts are handled — they are now assumed to contain JSON and formatted as such. The test seems to assume that the contents of these elements are treated literally (i.e. not interpolated) which is wrong. Will update the test.

@benmccann
Copy link
Member

We should ideally figure out why we now have simultaneous two versions of esbuild as of this PR.

Just FYI, we have two on main since adding better Cloudflare support recently because wrangler is stuck on an old version (cloudflare/workers-sdk#5222).

edit: And two versions of Rollup.

I sent #12118 to upgrade vite, vitest, rollup, and esbuild (minus the version being pulled in by wranger, which we can't do anything about)

* @param {string} code
*/
export function remove_self_closing_tags(code) {
return code.replace(/<([a-z-]+)(\s[^>]+?)? ?\/>/g, (match, name, attributes = '') => {
Copy link
Member

Choose a reason for hiding this comment

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

This is too broad:

  • doesn't leave foreign namespace alone
  • forces slot expansion even if those tags are not really real html tags, Svelte hijacks them to mean something different and as such self closing should be allowed. If we don't do that then I think the migration will produce a lot less noise for a piece of syntax that is deprecated anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

What foreign namespaces are we talking about? MathML? I'm not aware of any MathML elements that make sense without child content. Are people using other namespaces in Svelte components? The worst case scenario is we replace <valid /> with <valid></valid>. It would overcomplicate things to an absurd degree if we tried to avoid that.

Similarly, to distinguish between real <slot> elements and fake ones, we would need to determine whether a given .svelte component was intended to be compiled as a custom element, which... I don't know, I guess we would need to traverse the project looking for a svelte.config.js file (and miss cases where config was set directly via a bundler plugin) or something? And then we'd have <slot /> in some places and <slot></slot> in others and we'd need to explain why that was the case.

Seems like a bad use of time and energy!

Copy link
Member

@dummdidumm dummdidumm Apr 14, 2024

Choose a reason for hiding this comment

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

  • I'm talking about the namespace=foreign compiler option, which is used for Svelte Native etc. . Changing their code is just unnecessary.
  • For custom elements we just need to see if there's a customElement or tag option on svelte:options.
  • Finding a svelte.config.js is really easy and was done in various migration scripts before.
  • avoiding the slot thing will make the whole migration a lot less noisy and be accepted by more people.

Getting a migration script correct is a good spend of our time!
I can do the adjustments if you want to.

@Rich-Harris
Copy link
Member Author

closing in favour of #12128

@dummdidumm dummdidumm deleted the migrate-self-closing-tags branch April 16, 2024 20:50
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.

5 participants