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

[1.x] Add SSR support for Svelte 5 #1970

Merged
merged 15 commits into from
Sep 17, 2024
Merged

[1.x] Add SSR support for Svelte 5 #1970

merged 15 commits into from
Sep 17, 2024

Conversation

pedroborges
Copy link
Collaborator

@pedroborges pedroborges commented Sep 11, 2024

The primary goal for this PR is to maintain Svelte 4 as the default version in the adapter, ensuring stability for existing projects. However, I've also added support for Svelte 5, taking advantage of its backward compatibility with Svelte 4. This allows developers to upgrade their projects to Svelte 5 without needing to modify the adapter or deal with any breaking changes.

TL;DR:
The adapter remains on Svelte 4, but you can use either Svelte 4 or Svelte 5 in your projects seamlessly.

In Svelte 5 users no longer need to explicitly enable client side hydration in their vite.config.js file:

+     svelte(),
-     svelte({
-       compilerOptions: {
-         hydratable: true,
-       },
-     }),

@pedroborges pedroborges added the svelte Related to the svelte adapter label Sep 11, 2024
@pedroborges
Copy link
Collaborator Author

pedroborges commented Sep 11, 2024

@jamesst20, please help us test this PR if you can spare a moment 😉

@@ -20,6 +20,6 @@
"noUnusedLocals": true,
"noUnusedParameters": true,
"preserveConstEnums": true,
"removeComments": true
"removeComments": false
Copy link
Collaborator Author

@pedroborges pedroborges Sep 12, 2024

Choose a reason for hiding this comment

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

Needed to preserve the /* @vite-ignore */ comment that prevents Vite from statically analyzing and bundling svelte/server.

@@ -31,21 +31,21 @@
"!dist/**/*.spec.*"
],
"peerDependencies": {
"svelte": "^3.20.0 || ^4.0.0"
"svelte": "^3.20.0 || ^4.0.0 || ^5.0.0 || ^5.0.0-next.244"
Copy link
Collaborator Author

@pedroborges pedroborges Sep 12, 2024

Choose a reason for hiding this comment

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

Svelte 3.59.2, the latest version 3, was released over a year ago. We should consider whether it's worth continuing support for it. As far as I know, it still works with the current Svelte adapter, but with the upcoming v2.0 features and Svelte 5 on the horizon, we don't have the capacity to test across all versions to ensure full compatibility.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since this PR will be merged into Inertia 1.3, we'll maintain support for Svelte 3 for now and drop it with the release of Inertia V2.

Base automatically changed from new-ts-fixes to master September 12, 2024 13:39
@pedroborges pedroborges marked this pull request as ready for review September 12, 2024 14:01
@pedroborges pedroborges changed the title [Svelte] Add SSR support for Svelte 5 [2.x] Add SSR support for Svelte 5 Sep 13, 2024
@pedroborges pedroborges changed the title [2.x] Add SSR support for Svelte 5 [1.x] Add SSR support for Svelte 5 Sep 13, 2024
@jamesst20
Copy link
Contributor

jamesst20 commented Sep 16, 2024

@jamesst20, please help us test this PR if you can spare a moment 😉

Appears to be fine on my end! The approach of checking the Svelte version at runtime is the first approach I had taken in the past but for some reason it failed to work because the detected version at runtime was the one @inertiajs/svelte was built with instead of the version being used in the playground. I wonder where I failed. Not sure if this is the right commit in the history but it looked like this jamesst20@db54015#diff-ee8bc50b28e24eefd929600798a4a4c3283bd349a8bd968b0bef56c08271f628R7

I had opened a stackoverflow about it but couldn't find any solution at the time https://stackoverflow.com/questions/78470116/are-dynamic-imports-at-runtime-in-the-browser-esm-possible-with-svelte

It appears your implementation works as expected which is nice !

Edit

It appears the reason your way works and not mine is because Vite can't guess what the imported package is because you're doing it like this:

async function dynamicImport(modulePath: string) {
  try {
    return await import(/* @vite-ignore */ modulePath)
  } catch {
    return null
  }
}

but if it were like this you would end up having the same kind of issue I had

async function dynamicImport() {
  try {
    return await import(/* @vite-ignore */ 'svelte/server')
  } catch {
    return null
  }
}

@pedroborges
Copy link
Collaborator Author

pedroborges commented Sep 16, 2024

Thank you for testing it out, @jamesst20! Yes, passing the module path to dynamicImport was intentional—it was necessary after several failed attempts.

@jamesst20
Copy link
Contributor

Thank you for testing it out, @jamesst20! Yes, passing the module path to dynamicImport was intentional—it was necessary after several failed attempts.

Yeah this is crazy the right idea was there I just couldn't get it to work and yet it was so simple.

I wonder here jamesst20@b9a902d#diff-ee8bc50b28e24eefd929600798a4a4c3283bd349a8bd968b0bef56c08271f628R50 if I had changed

const { render } = await import('svelte/server')
return render(SSR, { props: { id, initialPage } })

to

const svelteImport = 'svelte/server'
const { render } = await import(svelteImport)
return render(SSR, { props: { id, initialPage } })

if this would also have worked. Anyway this is good news as breaking changes that relates to Inertia can be completely avoided

@jamesst20
Copy link
Contributor

Out of subject a little bit because I can see there are build errors. I had some troubles as well and was forced to move from NPM to PNPM at some point. I am not sure if this is what is happening here. From my testing I realized that when using NPM, the specified version in the packages.json from each "workspace" were not necessary always respected. I also realized that some of these workspaces were depending on some dependencies that were not explicitly declared in the packages.json but because some other project in the workspace were using them directly or indirectly from a subdepedency, it worked. If you are ever interested in taking a look, I had a PR opened to migrate to PNPM. Unfortunatly that PR was based on top of another one which updated every packages (#1874) but the relevent files are {packages,playgrounds}/**/package.json and pnpm-workspace.yaml and .npmrc and the github workflow itself. https://github.com/inertiajs/inertia/pull/1875/files#diff-0b810c38f3c138a3d5e44854edefd5eb966617ca84e62f06511f60acc40546c7

This had received 0 attention but I still think it was worth something #1873

@@ -17,7 +17,7 @@ jobs:
- name: Setup Node.js
uses: actions/setup-node@v2
with:
node-version: 16.15
node-version: 20.15
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Necessary because I got a ReferenceError: [plugin vite-plugin-svelte] structuredClone is not defined error on the build workflow while building the Svelte 5 playground.

@pedroborges pedroborges merged commit 2104a1b into master Sep 17, 2024
7 checks passed
@pedroborges pedroborges deleted the svelte-5-ssr branch September 17, 2024 19:51
pedroborges added a commit to olragon/inertia that referenced this pull request Sep 24, 2024
* master: (95 commits)
  [1.x] Fix props reactivity (inertiajs#1969)
  [1.x] useForm wrongly overwrites default values ​​after successful submission (inertiajs#1935)
  Update changelog
  [1.x] Fix `resetScrollPositions` and `restoreScrollPositions` router methods (inertiajs#1980)
  [1.x] Fix [scroll-region] on html element with overflow-scroll (inertiajs#1782)
  [1.x] Fix useForm re-renders by memoizing functions in React (inertiajs#1607)
  [1.x] Fix "DataCloneError: <Object> could not be cloned" (inertiajs#1967)
  [1.x] preserveScroll should be true on initial page visit (inertiajs#1360)
  Fix type augmentation (inertiajs#1958)
  [1.x] Fix doubling hash in React StrictMode (inertiajs#1728)
  [1.x] Add SSR support for Svelte 5 (inertiajs#1970)
  [1.x] Fix <Render /> component to respect "preserveState" (inertiajs#1943)
  [1.x] Fix 'received an unexpected slot "default"' warning (inertiajs#1941)
  QA: Add @types/lodash to fix svelte-check
  QA: Update reactive if statement
  Review useForm types
  QA: Move the if server up
  QA: Revert tsconfig change
  QA: Remove plural
  QA: Remove unused props type + add extra types just in case
  ...

# Conflicts:
#	packages/react/src/index.ts
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.

3 participants