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

Upgrade inertia to svelte 5 #1870

Closed
wants to merge 2 commits into from
Closed

Conversation

james-em
Copy link

@james-em james-em commented May 8, 2024

Upgrade Inertia to the new Svelte 5 API (runes).

@punyflash This will have some impact on #1866

@reinink Hopefully you merge typescript very soon and maybe I can update quickly this PR. I hope you could also release this quick under a @next tag for NPM

@punyflash
Copy link
Contributor

punyflash commented May 8, 2024

I believe this effort is unnecessary. Svelte 5 will maintain support for older APIs, so there's no compelling reason to discontinue support for Svelte 3 and 4. Additionally, the impact of transitioning to Svelte 5's API, which is based on signals, will likely be minimal for Inertia's components. For instance, App.svelte only observes global store values, while Render.svelte solely monitors component props. Consequently, migrating to the new APIs is unlikely to result in any significant speedup, if at all.

The only problem with Svelte 5 right now is SSR, which I believe should be rewritten in a same way as Vue and React adapters - all render logic should be exposed to developers in setup function on createInertiaApp

As an alternative, this could be implemented as a separate adapter, similar to how vue2 and vue3 are handled.

@james-em
Copy link
Author

james-em commented May 8, 2024

I believe this effort is unnecessary. Svelte 5 will maintain support for older APIs, so there's no compelling reason to discontinue support for Svelte 3 and 4. Additionally, the impact of transitioning to Svelte 5's API, which is based on signals, will likely be minimal for Inertia's components. For instance, App.svelte only observes global store values, while Render.svelte solely monitors component props. Consequently, migrating to the new APIs is unlikely to result in any significant speedup, if at all.

The only problem with Svelte 5 right now is SSR, which I believe should be rewritten in a same way as Vue and React adapters - all render logic should be exposed to developers in setup function on createInertiaApp

As an alternative, this could be implemented as a separate adapter, similar to how vue2 and vue3 are handled.

While I agree there is no huge performance difference, this is more about being up to date with Svelte 5 than supporting it through the legacy syntax. Svelte 3 is already no longer supported and I agree it still make sense to have Svelte 4 support but it doesn't necessary mean we should not ever move on to the new syntax just because we want to support previous major versions.

I believe having 2 adapters like you suggested is a great idea

@punyflash
Copy link
Contributor

Svelte 5 still supports old syntax, runes are introduced as a new feature of a compiler, you are not forced to use them, so why dive deeper into that adapter hell when you can just use what is working already? It will be hard to find people willing to keep both adapters up to date.

@james-em
Copy link
Author

james-em commented May 9, 2024

Svelte 5 still supports old syntax, runes are introduced as a new feature of a compiler, you are not forced to use them, so why dive deeper into that adapter hell when you can just use what is working already? It will be hard to find people willing to keep both adapters up to date.

You already said that and I already know that the old syntax is currently supported for now. Like I said I still don't think supporting Svelte 4 is a valid reason for staying on the old syntax. The old syntax is supported to give developers time to migrate. I made a new PR with a new Svelte 5 adapter that includes TypeScript support as well mostly based on your PR.

Also I disagree that we will have hard time finding contributions. I believe Inertia has many contributors and would have so much more if only it had active mainteners. I currently thinking on forking it and taking it whole because the way it is now this project can only die and this would be sad.

Svelte 4 adapter should be removed in a year or 2 at most. There is no reason to keep old versions alive for so long. If people aren't interested in updating Svelte, then they are likely not interested in updating Inertia either.

Also, I believe VueJS 2 should be dropped as it is way long dead and currently not supporting anything above NodeJS 18, and to say the truth, NodeJS 18 just work by luck. Thing is, all integrations tests are currently based on the VueJS 2 so this would need to be updated prior.

@reinink
Copy link
Member

reinink commented May 9, 2024

So from my perspective if we can at all avoid having two different Svelte adapters that would be preferred. Having two adapters for Vue is a pain, but unfortunately necessary at the time.

Meaning it would be nice if we could support Svelte 4 (and maybe even 3?) at the same time, even if we have to use a less-than-perfect approach for the time being until Svelte 5 becomes the defacto version, that would be my preference.

@james-em
Copy link
Author

Closing the PR since it doesn't support older version of Svelte and there is no interest in upgrading it fully for now by Inertia.

Note: I tweaked the types a little in my other PR that has its own Svelte adapter so this one is a little bit behind. I will be closing it too

@james-em james-em closed this May 10, 2024
@jamesst20
Copy link
Contributor

jamesst20 commented May 16, 2024

@punyflash

Turns out new Svelte 5 super reactive state do not work if the library itself ain't using runes even when using in a runes context from an app.

I have been working on a new app using my own Svelte 5 adapter and when switching to my minimalist version #1872 here I realised some states were not getting updated when using .push(...). However, under Svelte 5 runes they should. This is a huge drawback. Just though I would let you know.

Also, I went ahead and forked the repo and pushed many updates

https://www.npmjs.com/~jamesstp20
https://github.com/jamesst20/inertia/commits/main/

Svelte 5 adapter has its own branch https://github.com/jamesst20/inertia/commits/svelte5-adapter-playground

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.

4 participants