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

Svelte 5 POC #561

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Svelte 5 POC #561

wants to merge 5 commits into from

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Jan 10, 2024

This is a POC of migrating a small but more involved codebase to Svelte 5.

Notes

  • after bumping to Svelte 5 the app worked without any changes, though it has a few small bugs (see next section)
  • wanted $derived.fn a couple of times because of big derivations (ex: Folder.svelte)
  • wanted $effect.pre to run on the server, too, when converting some $: statements that were not reactive assignments
  • being more familiar with runes syntax now I find the result more readable. Especially the differentiation of $derived and $effect is very helpful to understand better what kind of code I'm dealing with
  • most migrations are generic, having automigration will help, it's rare that a change in behavior is introduced. $props() is the most tedious one (but also the easiest to automigrate)
  • the "synchronize from props but also update from above" situation occured in Menu.svelte
  • the "this needs to be $state" warning was very helpful in converting the right variables / not forgetting it
  • the "State referenced in its own scope will never update" warning was sometimes overzealous, for example when passing a proxied state to context when I know it's only the properties that are meant to be used reactively. But I think it will help greatly in other situations, so it's probably fine
  • converting stores to state objects required a bit of per-case thinking because the whole object isn't reactive, only its parts are. With writable there was a fixed contract making you think less about this because the container was already there. I ended up moving most separate writables into one big $state object because I don't have the "how to encapsulate it" problem then. This feels natural for state that belongs together but for primitive state you always have to think a bit how to do it. Usage-wise it didn't feel much different to stores, although doing x.set(newValue) feels a bit nicer compared to container.x = newValue, which feels hacky because you're mutating stuff all over. That could just be me though, and in a certain way it might even be good because the hackyness of modifying stuff from everywhere without hiding it behind a proper method that changes things is more obvious that way - though it isn't as easy to forbid writing to the state because there's no way to mark something as readonly for the outside but have it be regular $state within the same scope.
  • migrating towards event attributes was straightforward. In some cases it revealed workarounds (the payload being an object with one property on it, so that you can write e.detail.theName to know what you're dealing with which can now be simplified to just theName). In one case I had to add $props() because of event forwarding, which felt a bit more boilerplatey to write but afterwards it's much clearer what the component's in/outputs are because I can just scan for the $props() rune. In that instance I also had to add the children prop because it was receiving <slot /> content which was a bit confusing for a moment, maybe language tools can make it more clear what's going on there. Speaking of: I found the syntax highlighting to be worse, we should maybe highlight the attributes starting with on differently, and we definetly should add semantic highlighting to them.

Overall the migration was pretty straightforward, with a few gotchas which depending on your app could happen more or less frequently. The new code reads better overall to me, though not really much has changed, especially in the template (which is a good thing I'd say!)

Bugs

  • SvelteKit: afterNavigate doesnt fire initially, because onMount is async now (result: editor doesn't show code contents for initial load); we need to call flushState() in the backwards compatibility wrapper or in SvelteKit
  • folder tree: new file -> doesn't autofocus
  • Svelte/SvelteSplitPane: resizing doesn't work properly. In Svelte 4, bind:this triggers an update that is flushed in the next tick, in Svelte 5 it's synchronous. This results in one update in Svelte 4 with height/width set but two updates in Svelte 5 with the first having no width/height set yet, resulting in a too low calculation and therefore rendering the editor too small. I don't think we can fix this in Svelte 5 due to the timings, so the fix needs to happen in SvelteSplitPane (arguably it revealed a proper bug in the library)
  • Svelte: The file toggle doesn't work anymore when using $state instead of a writable, I don't know why. It's proxied state, so its parts should be reactive

Copy link

vercel bot commented Jan 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
learn-svelte-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 10, 2024 3:24pm

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.

1 participant