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

Skip Vite resolve workaround on Vite 4.1+ or Svelte 4+ #622

Merged
merged 6 commits into from
Apr 19, 2023

Conversation

benmccann
Copy link
Member

according to @dominikg:

latest vite does not need the custom resolve for it in v-p-s anymore, but early vite 4 versions did

I think we can safely remove this. People who are on old versions of Vite 4 may get larger apps if they upgrade vite-plugin-svelte without upgrading Vite, but that's relatively safe as nothing will break.

There's some nice benefits to removing this including simplifying the vite-plugin-svelte code and eventually paving the way for us to stop exporting svelte/ssr in Svelte core

@dominikg
Copy link
Member

dominikg commented Apr 14, 2023

This requires a changeset and is a breaking change as we would have to bump the peer dependency on vite.

Another option would be to import and test the vite version so we only keep doing it for the early vite-4 versions. That would then be a patch

@dominikg
Copy link
Member

dominikg commented Apr 14, 2023

Addendum: If you remove the svelte/ssr export that contains the onMount as noop, the feature would stop working altogether.

@benmccann
Copy link
Member Author

I can't currently run pnpm changeset, but will generate a changeset after #623 is merged.

I don't think it's a breaking change because nothing breaks if this is removed. The user just ends up with a slightly larger bundle, but that doesn't seem breaking to me.

Removing svelte/ssr would be breaking. I think the sooner we get this change in the better so that one day in the future when we do decide we want to remove svelte/ssr most users will have upgraded this library already.

@dominikg
Copy link
Member

slightly larger could be significantly larger and drive an edge deployment over a limit. I am not willing to take that risk.
If you want the standard resolve to be preferred, it would have to be with a major or fallback code for older vite-4

@benmccann
Copy link
Member Author

fallback code for older vite-4

Do you know what version of Vite 4 this was fixed in or have a link to the issue this was working around? I wouldn't know where to start looking

@bluwy
Copy link
Member

bluwy commented Apr 15, 2023

It's fixed in vitejs/vite#11595, which is released in Vite 4.1

@benmccann benmccann marked this pull request as draft April 16, 2023 01:18
@benmccann benmccann changed the title remove SSR resolve workaround Skip Vite resolve workaround on Vite 4.1+ Apr 18, 2023
@benmccann benmccann marked this pull request as ready for review April 18, 2023 23:34
@benmccann
Copy link
Member Author

I've updated this to only do the resolve workaround on Vite 4.0

@benmccann benmccann changed the title Skip Vite resolve workaround on Vite 4.1+ Skip Vite resolve workaround on Vite 4.1+ or Svelte 4+ Apr 19, 2023
@benmccann benmccann force-pushed the patch-4 branch 2 times, most recently from 725c502 to 8909c27 Compare April 19, 2023 15:22
@benmccann
Copy link
Member Author

I updated this to check the Svelte version as well

packages/vite-plugin-svelte/src/index.ts Outdated Show resolved Hide resolved
@dominikg dominikg merged commit 3185a36 into sveltejs:main Apr 19, 2023
@github-actions github-actions bot mentioned this pull request Apr 19, 2023
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.

3 participants