-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
🦋 Changeset detectedLatest commit: 61c07c2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
The lint failure here is caused by a battle between this and Prettier, which turns |
Fantastic, thank you! Updated the migration task to prompt people to upgrade — as soon as it's released, we'll bump the |
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. |
i have absolutely no idea |
downgrading |
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. |
I've taken the liberty of pushing 3148f19 - let's see how this looks now. |
I think that looks good? For posterity, what I did was grab an old copy of |
yeah, aside from the random failing test — investigating |
The failing test relates to this file:
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. |
Just FYI, we have two on
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 = '') => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
closing in favour of #12128 |
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:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits