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

Show file tree
Hide file tree
Changes from 6 commits
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
5 changes: 5 additions & 0 deletions .changeset/cold-pumpkins-enjoy.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/vite-plugin-svelte': patch
---

improve handling of transitive cjs dependencies of svelte libraries during dev ssr
2 changes: 1 addition & 1 deletion packages/vite-plugin-svelte/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export function svelte(inlineOptions?: Partial<Options>): Plugin {
// @ts-expect-error temporarily lend the options variable until fixed in configResolved
options = await preResolveOptions(inlineOptions, config, configEnv);
// extra vite config
const extraViteConfig = buildExtraViteConfig(options, config, configEnv);
const extraViteConfig = buildExtraViteConfig(options, config);
log.debug('additional vite config', extraViteConfig);
return extraViteConfig;
},
Expand Down
31 changes: 24 additions & 7 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 @@ -184,8 +184,7 @@ function resolveViteRoot(viteConfig: UserConfig): string | undefined {

export function buildExtraViteConfig(
options: PreResolvedOptions,
config: UserConfig,
configEnv: ConfigEnv
config: UserConfig
): Partial<UserConfig> {
// extra handling for svelte dependencies in the project
const svelteDeps = findRootSvelteDependencies(options.root);
Expand All @@ -200,7 +199,7 @@ export function buildExtraViteConfig(
// knownJsSrcExtensions: options.extensions
};

if (configEnv.command === 'serve') {
if (options.isServe) {
extraViteConfig.optimizeDeps = buildOptimizeDepsForSvelte(
svelteDeps,
options,
Expand All @@ -209,7 +208,7 @@ export function buildExtraViteConfig(
}

// @ts-ignore
extraViteConfig.ssr = buildSSROptionsForSvelte(svelteDeps, options, config);
extraViteConfig.ssr = buildSSROptionsForSvelte(svelteDeps, options, config, extraViteConfig);

return extraViteConfig;
}
Expand Down Expand Up @@ -294,7 +293,7 @@ function buildSSROptionsForSvelte(
// 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 @@ -309,13 +308,31 @@ function buildSSROptionsForSvelte(
// add svelte dependencies to ssr.noExternal unless present in ssr.external or optimizeDeps.include
noExternal.push(
...Array.from(new Set(svelteDeps.map((s) => s.name))).filter((x) => {
// @ts-ignore
// @ts-expect-error ssr still flagged in vite
return !config.ssr?.external?.includes(x) && !config.optimizeDeps?.include?.includes(x);
})
);
return {
const ssr = {
noExternal
};

if (options.isServe) {
// 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 || {})))
].filter(
(dep) =>
!ssr.noExternal.includes(dep) &&
// @ts-expect-error ssr still flagged in vite
!config.ssr?.noExternal?.includes(dep) &&
// @ts-expect-error ssr still flagged in vite
!config.ssr?.external?.includes(dep)
);
// @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;
}

export function patchResolvedViteConfig(viteConfig: ResolvedConfig, options: ResolvedOptions) {
Expand Down