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: provide migration function #11334

Merged
merged 22 commits into from Apr 26, 2024
Merged

feat: provide migration function #11334

merged 22 commits into from Apr 26, 2024

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Apr 25, 2024

Provides the start of a migration function, exported as migrate from svelte/compiler, which tries its best to automatically migrate towards runes, render tags (instead of slots) and event attributes (instead of event handlers)

The preview REPL was updated with a migrate button so people can try it out in the playground. Conveniently-chosen example that works nicely (click on migrate and see it happening)

closes #9239

TODO

  • figure out what do do for $: that don't get turned into $derived. Right now I'm just doing $effect but that's not equivalent because it doesn't run in SSR. Ideas:
    • expose an immediately-deprecated effect_or_once (I don't know how to call this) function which basically runs the code immediately on the server and as an effect on the client
    • make $effect.pre run on the server and use that (there's some issue somewhere that requests this because there are some code paths where this may be unavoidable to have an if-server-run-immediately-version)
    • duplicate the code and have an if-else written out in the migrated code
  • tests
  • handle more cases

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

Provides the start of a migration function, exported as `migrate` from `svelte/compiler`, which tries its best to automatically migrate towards runes, render tags (instead of slots) and event attributes (instead of event handlers)

The preview REPL was updated with a migrate button so people can try it out in the playground.

closes #9239
Copy link

changeset-bot bot commented Apr 25, 2024

🦋 Changeset detected

Latest commit: 9dc124b

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

@paoloricciuti
Copy link
Contributor

This is so beautiful 😍😍😍

@paoloricciuti
Copy link
Contributor

A couple of extra notes maybe:

  • $: console.log() could be turned into $inspect
  • derived are also writable in svelte 4... there's no way to translate that to svelte 5 so maybe add a comment or throw some error to fix?
  • // Todo will continue editing this comment if I found other possible enhancement

@Rich-Harris
Copy link
Member

expose an immediately-deprecated effect_or_once (I don't know how to call this) function which basically runs the code immediately on the server and as an effect on the client

Yeah, I think this might be the best option:

<script>
  import { run } from 'svelte/legacy';

  run(() => {
    // ...
  });
</script>

It'd be nice if we could avoid that in cases like this:

$: if (browser) {...}
$: if (typeof window !== 'undefined') {...}

@Rich-Harris
Copy link
Member

Very small detail but the 'migrate' button should be disabled/greyed out in runes mode

@paoloricciuti
Copy link
Contributor

expose an immediately-deprecated effect_or_once (I don't know how to call this) function which basically runs the code immediately on the server and as an effect on the client

Yeah, I think this might be the best option:

<script>
  import { run } from 'svelte/legacy';

  run(() => {
    // ...
  });
</script>

It'd be nice if we could avoid that in cases like this:

$: if (browser) {...}
$: if (typeof window !== 'undefined') {...}

Why exporting run from legacy? I feel like it could be a good helper in some occasions.

@dummdidumm
Copy link
Member Author

Very small detail but the 'migrate' button should be disabled/greyed out in runes mode

The migration also converts slots and event handlers, and you could use those in runes mode, too. What then?

@Rich-Harris
Copy link
Member

Ah, true. Perhaps it should only be greyed out if we can somehow detect that it wouldn't do anything.

Why exporting run from legacy? I feel like it could be a good helper in some occasions

What occasions?

@paoloricciuti
Copy link
Contributor

Ah, true. Perhaps it should only be greyed out if we can somehow detect that it wouldn't do anything.

Why exporting run from legacy? I feel like it could be a good helper in some occasions

What occasions?

I think sveltekit had one of those situations right? Where the solution was to duplicate the code once in the script tag and once in the effect? In general everywhere you want to setup something during SSR but also have it react once hydrated. Maybe I'm wrong tho, I hope to use as few effects as possible lol

@dummdidumm
Copy link
Member Author

Yeah whenever you interact with things you need to prepare imperatively before the dom is SSRd/rendered you need it (in SvelteKit it's initializing stores). Once everything's runes in your code the occasions definitely become less but they're still there.

let { onclick, ontoggle, 'custom-event-bubble': oncustom_event_bubble } = $props();
</script>

<button onclick={(payload) => {
Copy link
Member

Choose a reason for hiding this comment

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

maybe event rather than payload? any idea where the extra space is coming from?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's because there were multiple on:click handlers and their whitespaces where not removed. I think it's rare enough that this situation even happens to not care (and let prettier handle it; I like your suggestion of running prettier on the result within svelte-migrate)

@Rich-Harris
Copy link
Member

Would it be crazy to run Prettier on the output, if it (and prettier-plugin-svelte) can be found locally?

@Conduitry
Copy link
Member

@Rich-Harris That feels crazy to me. Nothing else in the Svelte compiler itself is filesystem-y currently, is it? I'd really like to keep these as close to pure functions as we can.

@Rich-Harris
Copy link
Member

I think it'd happen in the svelte-migrate package which calls the migrate function from here, since that's where all the filesystem-y stuff happens

@Conduitry
Copy link
Member

Oh okay, completely different repo. I have much fewer qualms about that, then.

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.

Implement an Upgrader for migrating from Svelte 4 to 5
4 participants