search

sveltejs / vite-plugin-svelte

Svelte plugin for http://vitejs.dev/
MIT License
844 stars 103 forks source link

chore: allow Vite 5 #817

Closed benmccann closed 10 months ago

benmccann commented 10 months ago

https://github.com/sveltejs/kit/pull/10818 is very complicated and a breaking change. This would be a much simpler solution. We can bump the dependency version in a couple of months to move everyone to vite-plugin-svelte 3 when we release SvelteKit 2

We don't have to fix any bugs in v2. We can still fix them just in v3 and tell non-SvelteKit users to upgrade to v3 and tell SvelteKit users they'll get any fixes in a couple of months. If there are any major issues that arise that need to be backported I can send PRs for it, I'm not too worried about that as vite-plugin-svelte has been very stable and battle tested already.

dominikg commented 10 months ago

not a fan of this. if you volunteer to maintain v2 and add vite5 to the test matrix it can work, but kit needs more changes to be compatible ( the manifest changes for example )

dominikg commented 10 months ago

also vite5 uses newer majors for esbuild and rollup, adapters and svelte-package could be affected too

benmccann commented 10 months ago

Sure, I'm happy to maintain v2

I'm trying to figure out I'd add Vite 5 to the test matrix. I think it'd have to be in a separate PR targeting the main branch? Because I see in main a reference to the v2 branch: https://github.com/sveltejs/vite-plugin-svelte/blob/63fafd4e5661425c088678daa1a16103daad6f22/.github/workflows/ci.yml#L8 So I'm assuming GitHub only runs the file located in main and the version in the v2 branch doesn't do anything. But perhaps we could wait until future changes are needed on the v2 branch? We already know that no changes were needed to vite-plugin-svelte for Vite 5 support when we updated main to Vite 5.

I'm not sure what needs to change in Kit regarding the manifest, but those changes would need to happen regardless. I'll bump esbuild and rollup in the adapters, etc. so that folks who are current only need a single version of them pulled into their project

dominikg commented 10 months ago

no, i think it has to be in this branch (the merge target). on main, there is a preparation to add svelte5 to the test matrix, you can use that as a template how to add vite 5 in this PR https://github.com/sveltejs/vite-plugin-svelte/blob/63fafd4e5661425c088678daa1a16103daad6f22/.github/workflows/ci.yml#L81

bluwy commented 10 months ago

I'm wondering another trick: Could we have v-p-s both as a dep and an optional peer dep in SvelteKit? That way if the user wants to use Vite 5, they can install v-p-s v3 and SvelteKit can pick that up through the peer dep; If they stick with Vite 4, it'll fallback to SvelteKit's normal v-p-s v2 dep. I have not tested it though.

Personally, I'm fine with relaxing the peer dep range only and not needing to tweak the CI and tests. We can say "we allow vite 5 only temporarily for svelte kit, but it's not something we guarantee. use at your own risk."

dominikg commented 10 months ago

we need the tests. if we do this, other setups can use v-p-s@2 + vite@5 as well, not just sveltekit. I'd prefer a solution on the sveltekit side aswell. That being said, if the work to maintain this is considered less than the work on sveltekit side, happy to accomodate - as long as it is treated as actual support -. We should also be clear that active support for v-p-s@2 is going to end soon after kit is able to use v-p-s@3, even if thats in a new major.

benmccann commented 10 months ago

I get really nervous about having two versions of v-p-s in a kit project. How will users know they need to install v-p-s themselves? Will npm, pnpm, yarn, etc. all handle it the same way? Will it make it harder to tell what version a user is on for bug reports? It just seems like an extra complication that we can easily avoid

I'm also not convinced the CI matrix is truly necessary, but at this point it has been added and there's certainly no harm in it. It is marginally safer, so I feel we might as well go with it since it's done.

In terms of support, I'd like to support it more than "use at your own risk". I do think that non-SvelteKit users could be lightly encouraged to upgrade though. E.g. I think it would be fine to say we're only going to backport large bug fixes (e.g. anything we discover related to Vite 5 support) and users should upgrade if they want fixes for edge cases that are unique to their projects.

benmccann commented 10 months ago

Closing given our latest discussions