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

dominikg · 2022-02-28T22:28:06Z

i'm still getting an error though when adding bytemd to a sveltekit project

this is rendered in the html output of localhost:3000 after a brief flash of an unstyled editor.

500
The requested module '/node_modules/.pnpm/codemirror-ssr@0.0.6/node_modules/codemirror-ssr/mode/gfm/gfm.js?v=9e01fcfd' does not provide an export named 'default'
SyntaxError: The requested module '/node_modules/.pnpm/codemirror-ssr@0.0.6/node_modules/codemirror-ssr/mode/gfm/gfm.js?v=9e01fcfd' does not provide an export named 'default'

benmccann · 2022-02-28T22:54:25Z

I was thinking that this is probably something that should be fixed purely in Vite. Vite is already including the packages in externals, but it's just that they're listed as parent > child so either we should make it understand that syntax or stop generating that syntax in that code path

dominikg · 2022-03-01T08:53:35Z

totally fine with fixing it in vite 👍 anyone got an idea why it's failing for codemirror-ssr though?

benmccann · 2022-03-01T13:59:30Z

I didn't look at that one yet, but codemirror is probably the hardest library I've ever encountered to get working in Vite, so I'm not terribly surprised or worried about that one. I shared with the bytemd how we got the main codemirror package working for svelte.dev

bluwy

Ben made vitejs/vite#7154 which should also fix this, but I think we should also merge this one as when someone turns on prebundleSvelteLibraries, they would hit this issue too.

bluwy · 2022-03-14T05:22:39Z

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

+	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
+			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))
+			);
+		}
+	}


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.

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

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 😵

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

benmccann · 2022-03-14T13:59:30Z

I wonder if we shouldn't focus on getting Vite to externalize everything by default. I took a stab at that earlier (vitejs/vite#6698), but one of the tests was failing and I didn't have time to investigate.

dominikg · 2022-03-14T22:02:09Z

is externalizing everything ok? if that was the case, why is vite doing this dance with noExternal in the first place?

benmccann · 2022-03-14T22:24:53Z

I believe it should be okay. At this point, I think the dance really isn't necessary and is just legacy that no one's taken the time to cleanup yet. Pretty much everything is already externalized and I'm not sure it would have any user visible changes. I think it's really just a single test that needs to be removed or updated. The failing test is optimize-missing-deps, which I'm really confused about because I'm not sure how anything can happen one way or another to a dependency that's missing. I don't really understand what it's supposed to be testing. At most I think we'd just need to special case missing dependencies and everything else can be externalized

bluwy · 2022-03-15T07:15:27Z

I think it would be great if we can find a way to externalize everything by default. I checked the failing test and it seems like Jest is interfering it with it's own transformation. Perhaps the CJS/ESM check Vite has avoided this specific edge case, but it looks like a Jest issue to me if things are working locally without Jest. I haven't looked too deep yet.

benmccann · 2022-03-16T00:27:54Z

Ah, if Jest was doing a transform that would explain a lot because it looked like I was getting code that was different than what was on disk. Another thing I noticed is that one of the package.json files in that test has ESM code, but is missing "type": "module"

…ies during dev

…rom generated list of externals

bluwy

LGTM

benmccann · 2022-03-16T16:27:15Z

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

+			...new Set(svelteDeps.flatMap((dep) => Object.keys(dep.pkg.dependencies || {})))
+		];
+		// @ts-expect-error ssr still flagged in vite
+		ssr.external = transitiveDepsOfSvelteLibs;


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

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.

dominikg added 2 commits February 28, 2022 23:25

fix: externalize transitive cjs only deps during dev ssr

f5bfcd2

fix: kindly tell ts we know about ssr

78e65f0

bluwy mentioned this pull request Mar 4, 2022

fix: ssrExternal should not skip nested dependencies vitejs/vite#7154

Merged

9 tasks

bluwy reviewed Mar 14, 2022

View reviewed changes

benmccann reviewed Mar 14, 2022

View reviewed changes

packages/vite-plugin-svelte/src/utils/options.ts Outdated Show resolved Hide resolved

fix: move generating external

681002c

dominikg added 3 commits March 16, 2022 14:33

fix: unconditionally externalize all transitive deps of svelte librar…

648cbf9

…ies during dev

fix: omit dependencies present in noExternal or already in external f…

af6b31b

…rom generated list of externals

fix: formatting and null checks

3b6b21d

bluwy approved these changes Mar 16, 2022

View reviewed changes

chore: tidy up

b7fe8ab

bluwy approved these changes Mar 16, 2022

View reviewed changes

dominikg merged commit 3d22f89 into main Mar 16, 2022

dominikg deleted the fix/externalize-cjs-ssr branch March 16, 2022 14:28

github-actions bot mentioned this pull request Mar 16, 2022

Version Packages (next) #297

Merged

benmccann reviewed Mar 16, 2022

View reviewed changes

github-actions bot mentioned this pull request Jul 13, 2022

Version Packages #390

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dominikg commented Feb 28, 2022

benmccann commented Feb 28, 2022

dominikg commented Mar 1, 2022

benmccann commented Mar 1, 2022

bluwy left a comment

bluwy Mar 14, 2022

dominikg Mar 14, 2022

bluwy Mar 15, 2022

benmccann commented Mar 14, 2022

dominikg commented Mar 14, 2022

benmccann commented Mar 14, 2022

bluwy commented Mar 15, 2022

benmccann commented Mar 16, 2022

bluwy left a comment

benmccann Mar 16, 2022

bluwy Mar 16, 2022

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

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

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

Conversation

dominikg commented Feb 28, 2022

benmccann commented Feb 28, 2022

dominikg commented Mar 1, 2022

benmccann commented Mar 1, 2022

bluwy left a comment

Choose a reason for hiding this comment

bluwy Mar 14, 2022

Choose a reason for hiding this comment

dominikg Mar 14, 2022

Choose a reason for hiding this comment

bluwy Mar 15, 2022

Choose a reason for hiding this comment

benmccann commented Mar 14, 2022

dominikg commented Mar 14, 2022

benmccann commented Mar 14, 2022

bluwy commented Mar 15, 2022

benmccann commented Mar 16, 2022

bluwy left a comment

Choose a reason for hiding this comment

benmccann Mar 16, 2022

Choose a reason for hiding this comment

bluwy Mar 16, 2022

Choose a reason for hiding this comment