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: Minimalest upgrade #1872

Closed
wants to merge 3 commits into from

Conversation

jamesst20
Copy link
Contributor

@jamesst20 jamesst20 commented May 10, 2024

  • Upgraded playground to Svelte 5
  • Fixed SSR breaking changes
  • packages.json: Allow Svelte 5 installation
  • Improvements: Render CSS properly for Svelte 3 & 4 include component CSS in head #1761
  • Svelte 5 conventions used: None

Half-Breaking change

Svelte 5 changed how SSR rendering works. Under Svelte 3&4, you can call MyComponent.render.
Under Svelte 5 you must import the render method from a package that doesn't exist under Svelte 3 & 4 making impossible to have compatibility for both unless we extract the component rendering outside.

I have updated the setup function to return the rendered result when in SSR mode.

// ssr.js
createServer((page) =>
  createInertiaApp({
    page,
    resolve: (name) => { ... },
    setup({ App, props }) {
      // Svelte 4: return App.render(props)
      // Svelte 5:
      return render(App, { props })
    },
  }),
)

I have added a warning when the implementation is not ok.

throw new Error(`setup(...) must return rendered result when in SSR mode.`)

I call this an half-breaking change because Svelte 4 apps will not break. I added a fallback with warning:

    if (!result && typeof (SSR as SSRComponentType).render !== 'function') {
      throw new Error(`setup(...) must return rendered result when in SSR mode.`)
    } else if (!result) {
      console.warn('Deprecated: in SSR mode setup(...) must be defined and must return rendered result in SSR mode.')
      console.warn('For Svelte 5: `return render(App, { props })` or for Svelte 4: `return App.render(props)`')
      result ||= (SSR as SSRComponentType).render!({ id, initialPage })
    }

Warning to Svelte 5 users

  • You still need to use $ syntax when importing reactive state from Inertia such as $page
  • Super reactive state will not work! Meaning $form.something.push({...}) will not trigger state refresh. This is likely a really huge bummer so you may consider using my fork adapter instead @jamesst20/inertia-svelte5. Hopefully Inertia maintainers change their mind about having a separate Svelte 5 adapter.
  • You still need to do stuff like $form.something = $form.something to trigger state refresh on updated object without creating a full copy before updating even if you use Svelte 5

jamesst20 added a commit to jamesst20/inertia that referenced this pull request May 12, 2024
jamesst20 added a commit to jamesst20/inertia that referenced this pull request May 12, 2024
jamesst20 added a commit to jamesst20/inertia that referenced this pull request May 12, 2024
jamesst20 added a commit to jamesst20/inertia that referenced this pull request May 12, 2024
jamesst20 added a commit to jamesst20/inertia that referenced this pull request May 13, 2024
jamesst20 added a commit to jamesst20/inertia that referenced this pull request May 13, 2024
jamesst20 added a commit to jamesst20/inertia that referenced this pull request May 13, 2024
jamesst20 added a commit to jamesst20/inertia that referenced this pull request May 16, 2024
jamesst20 added a commit to jamesst20/inertia that referenced this pull request May 28, 2024
jamesst20 added a commit to jamesst20/inertia that referenced this pull request May 28, 2024
@punyflash
Copy link
Contributor

punyflash commented Jun 14, 2024

I think it would be more reasonable to make it in a same way as Vue and React adapters works: just expose SSR component instead of App in setup function in case app is server-rendered and let user return rendered content:

createServer(page => createInertiaApp({
    page,
    resolve: name => resolvePageComponent(`./pages/${name}.svelte`, import.meta.glob('./pages/**/*.svelte')),
    setup: ({App, props}) => render(App, { props })
    // // Svelte 4
    // setup: ({App, props}) => App.render(props)
}))

Extract rendering outside createInertiaApp to allow Svelte 4 & 5 compatibility
@jamesst20
Copy link
Contributor Author

jamesst20 commented Jun 18, 2024

I think it would be more reasonable to make it in a same way as Vue and React adapters works: just expose SSR component instead of App in setup function in case app is server-rendered and let user return rendered content:

createServer(page => createInertiaApp({
    page,
    resolve: name => resolvePageComponent(`./pages/${name}.svelte`, import.meta.glob('./pages/**/*.svelte')),
    setup: ({App, props}) => render(App, { props })
    // // Svelte 4
    // setup: ({App, props}) => App.render(props)
}))

It wouldn't make sense to make the API exactly like React / Vue because then we should define a render property that would take care of the rendering and the setup would serve only one purpose and it would be to return App which createInertiaApp already own. Also, the render signature is different between Svelte 4 and 5 and the one from v4 comes from the component itself App.render.

However, I 100% agree this could be simplified to setup(...) only definition. I find it odd to have the same method in SSR/Non-SSR mode that servers 2 different purposes but it seems it's the way it currently is for every adapter so I applied your recommendations.

@reinink

Before I go ahead and update all of my PRs, getting this one merged or approved prior would be great! :)
Also, the build is now failing with this error

[vite-plugin-svelte] [plugin vite-plugin-svelte] structuredClone is not defined

I guess the CI setup is simply too old and this is already addressed by #1874

@pedroborges pedroborges added the svelte Related to the svelte adapter label Aug 8, 2024
@pedroborges pedroborges self-assigned this Aug 8, 2024
return
}
if (isServer) {
if (!result && typeof (SSR as SSRComponentType).render !== 'function') {
Copy link

Choose a reason for hiding this comment

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

I think you can make a cleaner check using import { VERSION } from 'svelte/compiler'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the follow up, I will need to double check that but I believe I had tried and this import is either not available il all svelte major versions or only available in the NodeJS context while compiling/transpiling with Vite. Also the import is going to fetch for Inertia's version and not the user specified version in his project.

At first I wanted to make this "transparent" to the user and dynamically render the proper way a component based on that version but I was unsuccessful

https://stackoverflow.com/questions/78470116/are-dynamic-imports-at-runtime-in-the-browser-esm-possible-with-svelte

packages/svelte/src/lib/createInertiaApp.ts Outdated Show resolved Hide resolved
packages/svelte/src/lib/createInertiaApp.ts Outdated Show resolved Hide resolved
jamesst20 and others added 2 commits August 11, 2024 10:54
Co-authored-by: Jeremiasz Major <jrh.mjr@gmail.com>
Co-authored-by: Jeremiasz Major <jrh.mjr@gmail.com>
@Sillyvan
Copy link

With 2.0 announced and not seeing a svelte tab in the presentation im wondering what the plan here is?

@pedroborges
Copy link
Collaborator

@Sillyvan I'm back maintaining the Svelte adapter. As soon as a couple open PRs are closed I'll be able focus my attention on bringing all the features planned for v2 to the Svelte adapter. That's the plan 👊

@Sillyvan
Copy link

@pedroborges very nice to hear. Is the plan still staying on stores?
From what we heard so far Svelte 6 will be the one that might remove compatibility.

So will the adapter just be updated when that is around the corner?
Maintainability wise this makes absolute sense we sadly miss out on some performance and some goodies but better than nothing

Thanks for your work

@pedroborges
Copy link
Collaborator

@Sillyvan The plan for V2 is to support both Svelte 4 and 5, so no runes for now. $page will continue to be a store but recently I investigated #1670 and found out that this issue is cause by the use of an internal store to hold component name and props that are passed down to layouts and page components. This will be fixed in V2.

I'm super exited about Svelte 5 and we will definitely use on future versions. It will make the Svelte adapter code much simpler for sure 🙌

@jamesst20
Copy link
Contributor Author

It appears to me the project Inertia is dead and there are no more reasons for me to keep my work open and maintain it.

I am closing all my opened PRs which includes #1864 #1872 #1873 #1874 #1875 #1881

I will be maintaining unofficially my own fork of this library so my projects using Svelte 5 can still live until I find or create an appropriate replacement for this library.

Thanks to everyone who spent time looking at my PRs.

@Sillyvan
Copy link

Sillyvan commented Sep 1, 2024

I totally get his frustration. The Svelte 5 PR ignored, how does he have 3 PRs fixing/improving open for 3+ months with 0 activity on it. He obviously cares a lot about this project

@incon
Copy link

incon commented Sep 2, 2024

@jamesst20 it's a shame seeing you giving up. In general, this package doesn't seem very friendly to outside contributions.

@reinink
Copy link
Member

reinink commented Sep 2, 2024

It appears to me the project Inertia is dead and there are no more reasons for me to keep my work open and maintain it.

I am closing all my opened PRs which includes #1864 #1872 #1873 #1874 #1875 #1881

I will be maintaining unofficially my own fork of this library so my projects using Svelte 5 can still live until I find or create an appropriate replacement for this library.

Thanks to everyone who spent time looking at my PRs.

Bummer dude 😕

We've been working really hard on Inertia.js v2.0 (as you can see in Taylor's Laracon keynote), so the project is far from dead — we're just in a bit of an awkward position between major versions.

I apologize that your work has not received more attention, that's partly my fault for not having enough time to review PRs recently. I don't work on Inertia.js full time like other open source maintainers (ie. Laravel, Livewire), so for me it actually requires time out of my personal life. Because of that things just take longer, as much as I wish it didn't.

Thanks for your contributions either way.

@pedroborges
Copy link
Collaborator

@jamesst20, I totally get where you're coming from, and I'm really sorry for the frustration you've been feeling. We can definitely do better here. I haven't been as involved as I wished. I was away from the project for the last two years, but I'm back now and really grateful for everyone who's kept contributing in the meantime 🙇

The team has been heads down working on Inertia.js v2, especially leading up to the Laracon announcement last week. There are a lot of cool new features coming, and I'm excited to be more involved again!

Just last Friday I mentioned internally that getting #1881 and #1872 reviewed and approved was my top priority this week since I'm blocked by them too. I'd really appreciate it if you could reopen these two PRs. After that, I'll be focusing on fixing #1670 and adding all the new v2 stuff to the Svelte adapter so we can launch it with the other adapters next month.

Thanks again for all your hard work and contributions. It really means a lot!

@jamesst20
Copy link
Contributor Author

@jamesst20, I totally get where you're coming from, and I'm really sorry for the frustration you've been feeling. We can definitely do better here. I haven't been as involved as I wished. I was away from the project for the last two years, but I'm back now and really grateful for everyone who's kept contributing in the meantime 🙇

The team has been heads down working on Inertia.js v2, especially leading up to the Laracon announcement last week. There are a lot of cool new features coming, and I'm excited to be more involved again!

Just last Friday I mentioned internally that getting #1881 and #1872 reviewed and approved was my top priority this week since I'm blocked by them too. I'd really appreciate it if you could reopen these two PRs. After that, I'll be focusing on fixing #1670 and adding all the new v2 stuff to the Svelte adapter so we can launch it with the other adapters next month.

Thanks again for all your hard work and contributions. It really means a lot!

I'm glad you are back ! :) I might re-contribute on the project later if I notice PRs ever starts to get merged again :)

Perhaps you could take a look at this issue? #775 (comment)

It's likely to affect more than Svelte 5 projects but short summary, when there are proxy in your variables (deep or not) structuedClone crashes and you can no longer page navigate. I'm still using the global workaround in my projects but it is ugly

@clementmas
Copy link

Svelte 5 is officially out now and looks very promising. Is there any progress being made now to support it?

@jamesst20
Copy link
Contributor Author

Svelte 5 is officially out now and looks very promising. Is there any progress being made now to support it?

@pedroborges Reopened a new pr and
Svelte 5 support was added by #1970

@clementmas
Copy link

Ah ok, it's available with the beta version (1.3 or 2.0). Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
svelte Related to the svelte adapter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants