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

Remove svelte/ssr resolving #176

Closed
wants to merge 1 commit into from
Closed

Conversation

benmccann
Copy link
Member

sveltejs/svelte#6743 is a better solution to this problem

@benmccann benmccann force-pushed the svelte-ssr branch 5 times, most recently from 8bc295a to 68abba8 Compare September 22, 2021 19:32
@dominikg
Copy link
Member

we need to update peer dependency on svelte to the version that adds the export map and I hope we can keep the current tests instead of removing them. They should still pass with the new solution

@Conduitry
Copy link
Member

Svelte 3.43.0 has been released with the new export map entry, so we can now try this out for real.

@benmccann benmccann force-pushed the svelte-ssr branch 2 times, most recently from aa00043 to 14bef29 Compare September 22, 2021 22:13
@bluwy
Copy link
Member

bluwy commented Sep 23, 2021

@benmccann We need to update the root package.json's svelte version as well since the pnpmfile enforces the same svelte version based on that. So technically the tests are still running 3.42.x.

We would also want to remove the ssr.noExternal handling here:

// add svelte to ssr.noExternal unless it is present in ssr.external
// so we can resolve it with svelte/ssr
if (options.isBuild && config.build?.ssr) {
// @ts-ignore
if (!config.ssr?.external?.includes('svelte')) {
noExternal.push('svelte');
}
} else {
// for non-ssr build, we exclude svelte js library deps to make development faster
// and also because vite doesn't handle them properly.
// see https://github.com/sveltejs/vite-plugin-svelte/issues/168
// see https://github.com/vitejs/vite/issues/2579
svelteDeps = svelteDeps.filter((dep) => dep.type === 'component-library');
}

but we could do it in a separate PR as this should also work as is. The changes in #166 can also be reverted, and refactored a bit to simplify things.

@benmccann benmccann force-pushed the svelte-ssr branch 2 times, most recently from 631d447 to 26f6ca4 Compare September 23, 2021 03:13
@@ -104,25 +101,6 @@ export function svelte(inlineOptions?: Partial<Options>): Plugin {
return importee; // query with svelte tag, an id we generated, no need for further analysis
}

if (ssr && importee === 'svelte') {
if (!resolvedSvelteSSR) {
resolvedSvelteSSR = this.resolve('svelte/ssr', undefined, { skipSelf: true }).then(
Copy link
Member

Choose a reason for hiding this comment

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

Since Vite can't resolve svelte to the node exports, I wonder if we should revert this file change, and swap out svelte/ssr to svelte, so that we "help" Vite out to resolve it. IIRC this.resolve, which uses the resolver plugin has support for exports map, so it should work. But I could completely wrong.

If so, I think we should left using svelte/ssr at the meantime

Copy link
Member

Choose a reason for hiding this comment

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

@benmccann Nevermind, tested this and it doesn't work

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.

None yet

4 participants