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

fix: improve setup of default export #8497

Closed
wants to merge 9 commits into from

Conversation

benmccann
Copy link
Member

closes #8088

@dominikg @bluwy do we still need to export ssr as well? Why does vite-plugin-svelte need the custom logic below? Shouldn't Vite resolve to the ssr or non-ssr version as appropriate using the default export?

https://github.com/sveltejs/vite-plugin-svelte/blob/f8d2bab37e875f654202baeb89d85af9c7501562/packages/vite-plugin-svelte/src/index.ts#L138

xxkl1 and others added 9 commits April 11, 2023 15:20
Instead of "both", which doesn't make sense at that point.
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
- upgrade to TypeScript 5
- upgrade @ampproject/remapping
- remove obsolete workarounds

---------

Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>
bump to rollup 3. Includes reworking the "treat those imports as external" a bit so that Rollup builds correctly but doesn't bundle some of the (now relative) imports

---------

Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>
@benmccann benmccann added this to the 4.x milestone Apr 12, 2023
@vercel
Copy link

vercel bot commented Apr 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
svelte-dev-2 ❌ Failed (Inspect) Apr 12, 2023 7:41pm

@vercel
Copy link

vercel bot commented Apr 12, 2023

@benmccann is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@Conduitry
Copy link
Member

I seem to recall there being some discussion where we decided the svelte/ssr stuff didn't actually help with treeshaking for SSR, or at least not in the way we were intending, but we couldn't remove it once it was added because it would be a breaking change. Does anyone remember what I might be thinking of?

@dominikg
Copy link
Member

To my knowledge this does work as intended, treeshaking imports only used in onMount during ssr.

latest vite does not need the custom resolve for it in v-p-s anymore, but early vite 4 versions did, so we cant remove it without breaking.

please keep the separate ssr export as is for now.
Also remind me to setup svelte ecosystem ci once the modern tooling is in place here.

@bluwy
Copy link
Member

bluwy commented Apr 13, 2023

I think we can remove the separate ssr export, that was because Vite isn't resolving the node condition correctly before, but it's now fixed. Since they can bump Vite to fix it, I don't think that should block us from removing it.

@dominikg
Copy link
Member

dominikg commented Apr 13, 2023

Oh, i didn't realize the browser condition above, thanks for pointing it out @bluwy

This would be a very breaking change for everything not understanding the browser exports condition. They got index.js before and now would get ssr.js

@dummdidumm
Copy link
Member

I vote for keeping the ssr export around a while longer, it's only a few lines in the package.json so doesn't hurt compared to the pain we would give people not knowing about this stuff

@benmccann
Copy link
Member Author

This would be a very breaking change for everything not understanding the browser exports condition. They got index.js before and now would get ssr.js

What falls into the category of understanding exports, but not browser? I've never seen such a tool

@dominikg
Copy link
Member

Any bundler not setting the browser condition explicitly, maybe unpkg or other package cdns? mjackson/unpkg#325

In principle I do agree with simplifying the conditions, and using "node" as synonymous for SSR isn't great.

Another question would be if rollup and webpack configs from users need to be updated to enable the browser condition, i'm not sure it is set by default there.

@benmccann
Copy link
Member Author

I think it's on by default for webpack:
https://webpack.js.org/guides/package-exports/#common-patterns
https://webpack.js.org/guides/package-exports/#target-environment

It's not for Rollup, but we suggest it in the rollup-plugin-svelte docs:
https://github.com/sveltejs/rollup-plugin-svelte#usage

@benmccann
Copy link
Member Author

svelte/ssr is a workaround for a bug in Vite 4.0 that was fixed in Vite 4.1. One option might be to remove svelte/ssr and release a new version of vite-plugin-svelte that uses it only when Svelte 3 is detected in the package.json. Then in the Svelte 4 release notes we can say it's only compatible with the newer version of vite-plugin-svelte which you can also get by bumping to a certain version of SvelteKit where we've bumped the vite-plugin-svelte version.

@benmccann
Copy link
Member Author

this branch is trashed. closing in favor of #8516

@benmccann benmccann closed this Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants