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.

Sign up for GitHub

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

Jump to bottom

fix: externalize transitive cjs only deps during dev ssr #289

Merged
merged 7 commits into from
Mar 16, 2022
Merged

fix: externalize transitive cjs only deps during dev ssr #289

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f5bfcd2
fix: externalize transitive cjs only deps during dev ssr
dominikg Feb 28, 2022
78e65f0
fix: kindly tell ts we know about ssr
dominikg Feb 28, 2022
681002c
fix: move generating external
dominikg Mar 14, 2022
648cbf9
fix: unconditionally externalize all transitive deps of svelte librar…
dominikg Mar 16, 2022
af6b31b
fix: omit dependencies present in noExternal or already in external f…
dominikg Mar 16, 2022
3b6b21d
fix: formatting and null checks
dominikg Mar 16, 2022
b7fe8ab
chore: tidy up
dominikg Mar 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 8 additions & 11 deletions packages/vite-plugin-svelte/src/utils/options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,15 +286,14 @@ function buildOptimizeDepsForSvelte(
function buildSSROptionsForSvelte(
svelteDeps: SvelteDependency[],
options: ResolvedOptions,
config: UserConfig,
extraViteConfig: UserConfig
config: UserConfig
): any {
const noExternal: string[] = [];

// 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
// @ts-expect-error ssr still flagged in vite
if (!config.ssr?.external?.includes('svelte')) {
noExternal.push('svelte');
}
Expand All @@ -318,14 +317,12 @@ function buildSSROptionsForSvelte(
};

if (options.isServe) {
// during dev, we have to externalize transitive cjs only dependencies, see https://github.com/sveltejs/vite-plugin-svelte/issues/281
const optimizedTransitiveDeps = extraViteConfig.optimizeDeps?.include
?.filter((x) => x.includes('>'))
.map((x) => x.substring(x.lastIndexOf('>') + 1).trim());
if (optimizedTransitiveDeps?.length) {
// @ts-expect-error ssr still flagged in vite
ssr.external = optimizedTransitiveDeps.filter((x) => !config.ssr?.noExternal?.includes(x));
}
// during dev, we have to externalize transitive dependencies, see https://github.com/sveltejs/vite-plugin-svelte/issues/281
const transitiveDepsOfSvelteLibs = [
...new Set(svelteDeps.flatMap((dep) => Object.keys(dep.pkg.dependencies || {})))
];
// @ts-expect-error ssr still flagged in vite
ssr.external = transitiveDepsOfSvelteLibs;
Copy link
Member

Choose a reason for hiding this comment

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

I wasn't suggesting to change anything here, but rather in Vite. I'm not entirely sure this is safe. I hope it is, but I can't figure out how to externalize all deps and get Vite's optimize-missing-deps test to pass, so there might be a real issue there around deps that haven't been optimized yet

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be safe to fix this here so it works in Vite 2.8 at the meantime. Having this handled in Vite 2.9 ootb would be great though so perhaps later on we can remove this.

}

return ssr;
Expand Down