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 2 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
18 changes: 18 additions & 0 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 @@ -211,6 +211,24 @@ export function buildExtraViteConfig(
// @ts-ignore
extraViteConfig.ssr = buildSSROptionsForSvelte(svelteDeps, options, config);

if (configEnv.command === 'serve') {
// 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 flagge in vite
dominikg marked this conversation as resolved.
Show resolved Hide resolved
if (!extraViteConfig.ssr.external) {
// @ts-expect-error ssr still flagge in vite
extraViteConfig.ssr.external = [];
}
// @ts-expect-error ssr still flagge in vite
extraViteConfig.ssr.external.push(
// @ts-expect-error ssr still flagge in vite
...optimizedTransitiveDeps.filter((x) => !config.ssr?.noExternal?.includes(x))
);
}
}
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 we could move this into buildSSROptionsForSvelte() and build the list of externals from scratch, since if prebundleSvelteLibraries is turned off, there won't be parent > child syntax to handle.

Copy link
Member Author

Choose a reason for hiding this comment

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

moved it and cleaned up. not sure i follow the prebundleSvelteLibraries problem.

Copy link
Member

Choose a reason for hiding this comment

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

If prebundleSvelteLibraries is set to true, buildOptimizeDepsForSvelte() will early return here:

vite-plugin-svelte/packages/vite-plugin-svelte/src/utils/options.ts

Lines 244 to 252 in 681002c

if (options.experimental.prebundleSvelteLibraries) {
return {
include,
exclude,
esbuildOptions: {
plugins: [{ name: facadeEsbuildSveltePluginName, setup: () => {} }]
}
};
}

Skipping the below code that handles adding parent > child to optimizeDeps.include so there won't be any > syntax to handle in configuring ssr.external later on, which brings up the initial issue where nested deps (e.g. child) weren't added to ssr.external when it should.

To showcase this visually with the current PR's changes:

If prebundleSvelteLibraries: false (default)

  • optimizeDeps.include: ["svelte-dep > cjs-dep"]
  • optimizeDeps.exclude: ["svelte-dep"]
  • ssr.noExternal: ["svelte-dep"]
  • ssr.external: ["cjs-dep"]

If prebundleSvelteLibraries: true

  • optimizeDeps.include: []
  • optimizeDeps.exclude: []
  • ssr.noExternal: ["svelte-dep"]
  • ssr.external: [] <- should have cjs-dep

So we have to reconstruct the ssr.external without relying on optimizeDeps.include 😵

return extraViteConfig;
}

Expand Down