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] Svelte 5 support #2288

Draft
wants to merge 3 commits into
base: 2.x
Choose a base branch
from
Draft

Conversation

ChqThomas
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
Issues /
License MIT

Svelte 5 was just released 🚀

  • Updated the peerDependencies of ux-svelte to support Svelte 5
  • The controller has been adapted to continue supporting versions 3 and 4 of Svelte
  • Some tests are only run on version 3/4, and some only on version 5
  • Tested on a webpack-encore project successfuly
  • Can't get it to work with an importmap (for asset-mapper) ATM, there is an opened issue on Svelte repo about this
  • note : @sveltejs/vite-plugin-svelte updated to v3 only instead on v4 because v4 removed support for Svelte < 5

@carsonbot carsonbot added Feature New Feature Svelte Status: Needs Review Needs to be reviewed labels Oct 20, 2024
@smnandre
Copy link
Member

Some reading here: https://svelte-5-preview.vercel.app/docs/breaking-changes

(just passing by)

@@ -37,6 +37,13 @@ class default_1 extends Controller {
};
this.dispatch(name, { detail, prefix: 'svelte' });
}
async mountSvelteComponent(Component, options) {
const { mount } = await import('svelte');
Copy link
Member

Choose a reason for hiding this comment

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

This will not work with AssetMapper right ? In which cas we need to find a solution because this would be a giant BC break :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my first thought but it worked when I tried it in ux.symfony.com !
I also tried to have import { mount } from 'svelte' at the top of the file but it doesn't work with AssetMapper with Svelte 4, an error was thrown because mount only exists in v5
I can't explain why but with a dynamic import there is no error, mount is just undefined

Copy link
Member

Choose a reason for hiding this comment

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

I'll test this next week, thanks for the explanations :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! FYI with AssetMapper it works with Svelte 4, but with Svelte 5 I'm stuck with the issue described here
I made a REPL with only html/js that reproduce the error

@smnandre
Copy link
Member

@Kocal Kocal self-requested a review October 31, 2024 06:49
@Kocal
Copy link
Member

Kocal commented Oct 31, 2024

I'm starting to investigate what's missing to make Svelte 5 happy here, just personal notes:

  • svelte 4.x sf console importmap:require svelte@4.2.19 svelte/internal@4.2.19 svelte/internal/disclose-version@4.2.19 && pnpm add -D svelte@4.2.19 && pnpm run build-svelte
  • svelte 5.x sf console importmap:require svelte svelte/internal/client svelte/internal/disclose-version && pnpm add -D svelte && pnpm run build-svelte@5.1.6
  • svelte version and packages in importmap definition will need to be updated

@Kocal Kocal marked this pull request as draft October 31, 2024 08:52
@ChqThomas
Copy link
Contributor Author

I'm starting to investigate what's missing to make Svelte 5 happy here, just personal notes:

  • svelte 4.x sf console importmap:require svelte@4.2.19 svelte/internal@4.2.19 svelte/internal/disclose-version@4.2.19 && pnpm add -D svelte@4.2.19 && pnpm run build-svelte
  • svelte 5.x sf console importmap:require svelte svelte/internal/client svelte/internal/disclose-version && pnpm add -D svelte && pnpm run build-svelte@5.1.6
  • svelte version and packages in importmap definition will need to be updated

Small tips: you can get rid of the svelte/internal/disclose-version dependency if you compile the Svelte components with the discloseVersion: false option - documentation

@Kocal
Copy link
Member

Kocal commented Oct 31, 2024

Ok I think I'm on something. I believe that's because the context between svelte and svelte/internal/client is not shared.

In one of the module, init_operations() is called and first_child_getter and next_sibling_getter are defined.
But, on the other module, which depends of first_child_getter and next_sibling_getter, calls to get_first_child (called by mount) fails because first_child_getter is undefined.

I believe this is related to no-build scenarios, like we do with AssetMapper/importmap.

I will investigate more and try to find a solution.

@smnandre
Copy link
Member

I believe this is related to no-build scenarios, like we do with AssetMapper/importmap.

I will investigate more and try to find a solution.

It is, and some people are trying weird stuff with chunks (i'm not sure to even understand what i'm saying here haha)

@Kocal
Copy link
Member

Kocal commented Oct 31, 2024

To be honest, due to how files are bundled on JSDelivr and how Svelte 5 works, I don't know if we can have Svelte 5 support for AssetMapper... :/

@smnandre
Copy link
Member

smnandre commented Oct 31, 2024

I'd be ok if the support was only for webpack :| But no BC break for AssetMapper please :)

But i don't get how this work as... in the end... there are only compiled assets in a directory right ?

@Kocal
Copy link
Member

Kocal commented Nov 1, 2024

But i don't get how this work as... in the end... there are only compiled assets in a directory right ?

Not with AssetMapper / CDN stuff.

When you compile this .svelte file for Svelte 5:

<script>
    export let name = "Svelte";
</script>

<div>Hello {name}</div>

It become:

import * as $ from "svelte/internal/client";

var root = $.template(`<div> </div>`);

export default function _unknown_($$anchor, $$props) {
	let name = $.prop($$props, "name", 8, "Svelte");
	var div = root();
	var text = $.child(div);

	$.reset(div);
	$.template_effect(() => $.set_text(text, `Hello ${name() ?? ""}`));
	$.append($$anchor, div);
}

Here, we have a dependency to svelte/internal/client, you can install it through importmap:require.

In the render_controller.ts file, we have a dependency to svelte to import mount and unmount functions (if they exists, in Svelte 5). You can also install svelte through importmap:require.

The thing is, that both dependencies svelte and svelte/internal/client contain both the same inlined (and that's the problem) logic (the function init_operations() instead of relying on a common module.

One dependency defines first_child_getter and next_sibling_getter function, and the other dependency tries to use those functions but without success, since the context is not shared across the two dependencies... :/

IMO, the best we can do is to:

  1. keep render_controller.js as-if, only support Svelte 3/4
  2. and either:
    1. since we can do BC breaks here, create a new UX package symfony/ux-svelte-5, without AssetMapper support, with good devDependencies versions, that's the easier and cleanest solution
    2. or a more and more complex solution with some drawbacks :
      1. add a new option svelte.comptability_version (default to 4),
      2. create a new Stimulus controller for Svelte 5,
      3. try to see how can we warn the user that nothing will work if you use the AssetMapper with svelte.comptability_version = 5, but I don't know how we can do that properly
      4. but, the importmap.php and package.json dependencies will be invalid because by default they contains deps for Svelte 3/4 :/

I'm more in favor of a new dedicated symfony/ux-svelte-5 package, as it will be more easier for us to work on, and more easier for users to use it.

@ChqThomas
Copy link
Contributor Author

The potential issue I see with creating a new ux-svelte-5 package now is that if Svelte fixes the CDN issue in a future release, we'll have to do a BC break to remove the new package and come back to the initial one

@smnandre
Copy link
Member

smnandre commented Nov 2, 2024

There is not much code in the package, i'm sure we can make something in it to load or or the other file :)

@Kocal
Copy link
Member

Kocal commented Nov 3, 2024

There is not much code in the package, i'm sure we can make something in it to load or or the other file :)

Yeah, but for the moment Svelte 5 only works with Webpack, not with AssetMapper. Even if we can do something "to load or not the other file", it still won't work :s or am I missing something?

@smnandre
Copy link
Member

smnandre commented Nov 3, 2024

Yeah, but for the moment Svelte 5 only works with Webpack, not with AssetMapper.

That's what i reacted on when i said "i'd be ok to no have AssetMapper support"

We can announce a compatibility for only Webpack no ?

@Kocal
Copy link
Member

Kocal commented Nov 4, 2024

Yes we can

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Needs Review Needs to be reviewed Svelte
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants