fix: wrap :id, :where:not and :has with :global during migration

[paoloricciuti]

Before submitting the PR, please make sure you do the following

Closes #13765...if you can think of more weird case to test please let me know.

EDIT: i've added a slightly better test and realised that we could have a problem if the user has something like this

{`
<style>
	.this :is(a .problem){}
</style>
`}

in the template...should we care?

Also weirdly replaceAll from MagicString did not got the job done, might be worth opening an issue there.

• 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
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

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

[changeset-bot]

🦋 Changeset detected

Latest commit: 60fb2b5

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

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

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

[dummdidumm]

just for safety: can we add more tests around nested braces? Like :has(.x:not(...))

[paoloricciuti]

Sure...if you can think of any other case please let me know

[dummdidumm]

div :is(.class:is(span:is(:hover)), .x){} -> the .x isn't properly wrapped

[paoloricciuti]

Oh I know why...let me fix

[dummdidumm]

should we care?

I don't think this will ever come up in the wild, so I'm good with this approach, assuming we can deal with :is(.x, .y) cases.

[paoloricciuti]

ouch i borked the fix...i need a bit more thinkering to fix this

[paoloricciuti]

I don't think this is doable with regex only...we might need to interveen with some JS. Let me see what i can do

[dummdidumm]

nice an simple

[Conduitry]

I'd still like to argue for providing a way to run only this migration (and other future migrations we write that are for upgrading Svelte 4 code to non-runes Svelte 5 code).

I'm also worried about idempotency and this sort of change. When we're migrating a component, how do we know whether the user already made the appropriate changes for non-runes mode? Adding :global()s might cause them to lose scoping that they intended to have and were already accounting for from the v5 changes.

[dummdidumm]

I'm not as worried about that since it's probably very rare that you'd escape scoping with one of these selectors. But yes, maybe we should do that change only when we detect it's not in runes mode yet? This is definitely tricky 😅

[Rich-Harris]

Surely the issue is that you write a scoped selector, run the migration (on your entire codebase, which happens to include this component that doesn't need migration), and now you have an unscoped selector?

I'd previously suggested skipping the migration altogether for components that are already in runes mode for basically this reason, though it wouldn't help in the case of an ambiguous

[dummdidumm]

skipping it entirely is a bit too harsh potentially because you might have done runes but not event attributes yet, which are harmless to do.

[pkg-pr-new]

pnpm add https://pkg.pr.new/svelte@13850

commit: 60fb2b5

[paoloricciuti]

I'd still like to argue for providing a way to run only this migration (and other future migrations we write that are for upgrading Svelte 4 code to non-runes Svelte 5 code).

I'm also worried about idempotency and this sort of change. When we're migrating a component, how do we know whether the user already made the appropriate changes for non-runes mode? Adding :global()s might cause them to lose scoping that they intended to have and were already accounting for from the v5 changes.

Oh this also made me remember...we probably want to skip this if it's already global

[paoloricciuti]

Considering that this migration is literally it's own separate function we could split it in a separate command (and maybe ask in the cli if you want to migrate css too)

[dummdidumm]

This feels like too much overhead - most people will not know how to answer such questions. People who run the migration on a component (or project) do so with the intention of getting it transformed in a way that mimicks Svelte 4 as closely as possible while upgrading the syntax. It's impossible to know whether or not the intention of something being run on a component already in runes mode is to automatically convert the rest (like @paoloricciuti would probably do on his project after this lands; if he didn't already do it by hand) or leave it alone. I vote for erroring on the side of "wants more" and always do it.

[paoloricciuti]

Yeah there's also to say that you still need to check the files and it's pretty easy to revert this changes with a click if you watch in diff mode in vscode