fix: configure Vite config optimizeDeps.entries option (#12182)

eltigerchino · web-flow · commit 7a625724e240 · 2024-05-04T20:42:53.000+08:00
* add failing test

* add optimizeDeps.entries

* changeset

* prettier

* only match + prefixed route files

* try to fix test for ubuntu firefox

* hope this works

* nicer comment...?

* move test to dev-only test app

* prettier

* oops forgot to undo this

* add tests for each routing file

* prettier
diff --git a/.changeset/stale-otters-yell.md b/.changeset/stale-otters-yell.md
@@ -0,0 +1,5 @@
+---
+"@sveltejs/kit": patch
+---
+
+fix: prevent excessive Vite dependency optimizations on navigation
diff --git a/packages/kit/src/exports/vite/index.js b/packages/kit/src/exports/vite/index.js
@@ -293,6 +293,7 @@ async function kit({ svelte_config }) {
 					cors: { preflightContinue: true }
 				},
 				optimizeDeps: {
+					entries: [`${kit.files.routes}/**/+*.{svelte,js,ts}`],
 					exclude: [
 						'@sveltejs/kit',
 						// exclude kit features so that libraries using them work even when they are prebundled
diff --git a/packages/kit/test/apps/dev-only/_test_dependencies/cjs-only/index.js b/packages/kit/test/apps/dev-only/_test_dependencies/cjs-only/index.js
@@ -0,0 +1,3 @@
+module.exports = {
+	cjs: () => 'cjs'
+};
diff --git a/packages/kit/test/apps/dev-only/_test_dependencies/cjs-only/package.json b/packages/kit/test/apps/dev-only/_test_dependencies/cjs-only/package.json
@@ -0,0 +1,10 @@
+{
+	"version": "1.0.0",
+	"private": true,
+	"name": "e2e-test-dep-cjs-only",
+	"main": "index.js",
+	"files": [
+		"index.js"
+	],
+	"type": "commonjs"
+}
diff --git a/packages/kit/test/apps/dev-only/package.json b/packages/kit/test/apps/dev-only/package.json
@@ -13,6 +13,16 @@
 		"@sveltejs/kit": "workspace:^",
 		"@sveltejs/vite-plugin-svelte": "^3.0.1",
 		"cross-env": "^7.0.3",
+		"e2e-test-dep-page-svelte": "file:./_test_dependencies/cjs-only",
+		"e2e-test-dep-page-universal": "file:./_test_dependencies/cjs-only",
+		"e2e-test-dep-page-server": "file:./_test_dependencies/cjs-only",
+		"e2e-test-dep-layout-svelte": "file:./_test_dependencies/cjs-only",
+		"e2e-test-dep-layout-universal": "file:./_test_dependencies/cjs-only",
+		"e2e-test-dep-layout-server": "file:./_test_dependencies/cjs-only",
+		"e2e-test-dep-server": "file:./_test_dependencies/cjs-only",
+		"e2e-test-dep-error": "file:./_test_dependencies/cjs-only",
+		"e2e-test-dep-hooks": "file:./_test_dependencies/cjs-only",
+		"e2e-test-dep-hooks-client": "file:./_test_dependencies/cjs-only",
 		"svelte": "^4.2.10",
 		"svelte-check": "^3.6.3",
 		"typescript": "^5.3.3",
diff --git a/packages/kit/test/apps/dev-only/src/hooks.client.js b/packages/kit/test/apps/dev-only/src/hooks.client.js
@@ -0,0 +1,7 @@
+import cjs from 'e2e-test-dep-hooks-client';
+cjs.cjs();
+
+/** @type {import("@sveltejs/kit").HandleClientError} */
+export function handleError({ message }) {
+	return { message };
+}
diff --git a/packages/kit/test/apps/dev-only/src/hooks.js b/packages/kit/test/apps/dev-only/src/hooks.js
@@ -0,0 +1,7 @@
+import cjs from 'e2e-test-dep-hooks';
+cjs.cjs();
+
+/** @type {import("@sveltejs/kit").Reroute} */
+export function reroute({ url }) {
+	return url.pathname;
+}
diff --git a/packages/kit/test/apps/dev-only/src/routes/+page.svelte b/packages/kit/test/apps/dev-only/src/routes/+page.svelte
@@ -0,0 +1,13 @@
+<script>
+	import { onMount } from 'svelte';
+
+	let message = '';
+
+	onMount(() => {
+		message = 'hello world!';
+	});
+</script>
+
+<p>{message}</p>
+
+<a href="/optimize-deps">Go to /optimize-deps</a>
diff --git a/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+error.svelte b/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+error.svelte
@@ -0,0 +1,8 @@
+<script>
+	import cjs from 'e2e-test-dep-error';
+	cjs.cjs();
+</script>
+
+<slot />
+
+<p>this error page uses a new dependency</p>
diff --git a/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+layout.js b/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+layout.js
@@ -0,0 +1,4 @@
+import cjs from 'e2e-test-dep-layout-universal';
+cjs.cjs();
+
+export function load() {}
diff --git a/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+layout.server.js b/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+layout.server.js
@@ -0,0 +1,4 @@
+import cjs from 'e2e-test-dep-layout-server';
+cjs.cjs();
+
+export function load() {}
diff --git a/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+layout.svelte b/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+layout.svelte
@@ -0,0 +1,8 @@
+<script>
+	import cjs from 'e2e-test-dep-layout-svelte';
+	cjs.cjs();
+</script>
+
+<slot />
+
+<p>this layout uses a new dependency</p>
diff --git a/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+page.js b/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+page.js
@@ -0,0 +1,4 @@
+import cjs from 'e2e-test-dep-page-universal';
+cjs.cjs();
+
+export function load() {}
diff --git a/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+page.server.js b/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+page.server.js
@@ -0,0 +1,4 @@
+import cjs from 'e2e-test-dep-page-server';
+cjs.cjs();
+
+export function load() {}
diff --git a/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+page.svelte b/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+page.svelte
@@ -0,0 +1,6 @@
+<script>
+	import cjs from 'e2e-test-dep-page-svelte';
+	cjs.cjs();
+</script>
+
+<p>this page uses a new dependency</p>
diff --git a/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+server.js b/packages/kit/test/apps/dev-only/src/routes/optimize-deps/+server.js
@@ -0,0 +1,6 @@
+import cjs from 'e2e-test-dep-server';
+cjs.cjs();
+
+export function GET() {
+	return new Response('ok');
+}
diff --git a/packages/kit/test/apps/dev-only/test/test.js b/packages/kit/test/apps/dev-only/test/test.js
@@ -1,5 +1,10 @@
 import { expect } from '@playwright/test';
 import { test } from '../../../utils.js';
+import fs from 'node:fs';
+import path from 'node:path';
+import { fileURLToPath } from 'node:url';
+
+const __dirname = path.dirname(fileURLToPath(import.meta.url));
 
 /** @typedef {import('@playwright/test').Response} Response */
 
@@ -78,3 +83,97 @@ test.describe.serial('Illegal imports', () => {
 		);
 	});
 });
+
+test.describe('Vite', () => {
+	test.skip(({ javaScriptEnabled }) => !process.env.DEV || !javaScriptEnabled);
+
+	test('optimizes +page.svelte dependencies', async ({ page }) => {
+		await page.goto('/');
+		await page.getByText('hello world!').waitFor();
+
+		const manifest_path = path.join(__dirname, '../node_modules/.vite/deps/_metadata.json');
+		const manifest = JSON.parse(fs.readFileSync(manifest_path, 'utf-8'));
+
+		expect(manifest).toHaveProperty('optimized.e2e-test-dep-page-svelte');
+	});
+
+	test('optimizes +page.js dependencies', async ({ page }) => {
+		await page.goto('/');
+		await page.getByText('hello world!').waitFor();
+
+		const manifest_path = path.join(__dirname, '../node_modules/.vite/deps/_metadata.json');
+		const manifest = JSON.parse(fs.readFileSync(manifest_path, 'utf-8'));
+
+		expect(manifest).toHaveProperty('optimized.e2e-test-dep-page-universal');
+	});
+
+	test('optimizes +page.server.js dependencies', async ({ page }) => {
+		await page.goto('/');
+		await page.getByText('hello world!').waitFor();
+
+		const manifest_path = path.join(__dirname, '../node_modules/.vite/deps/_metadata.json');
+		const manifest = JSON.parse(fs.readFileSync(manifest_path, 'utf-8'));
+
+		expect(manifest).toHaveProperty('optimized.e2e-test-dep-page-server');
+	});
+
+	test('optimizes +layout.svelte dependencies', async ({ page }) => {
+		await page.goto('/');
+		await page.getByText('hello world!').waitFor();
+
+		const manifest_path = path.join(__dirname, '../node_modules/.vite/deps/_metadata.json');
+		const manifest = JSON.parse(fs.readFileSync(manifest_path, 'utf-8'));
+
+		expect(manifest).toHaveProperty('optimized.e2e-test-dep-layout-svelte');
+	});
+
+	test('optimizes +layout.js dependencies', async ({ page }) => {
+		await page.goto('/');
+		await page.getByText('hello world!').waitFor();
+
+		const manifest_path = path.join(__dirname, '../node_modules/.vite/deps/_metadata.json');
+		const manifest = JSON.parse(fs.readFileSync(manifest_path, 'utf-8'));
+
+		expect(manifest).toHaveProperty('optimized.e2e-test-dep-layout-universal');
+	});
+
+	test('optimizes +layout.server.js dependencies', async ({ page }) => {
+		await page.goto('/');
+		await page.getByText('hello world!').waitFor();
+
+		const manifest_path = path.join(__dirname, '../node_modules/.vite/deps/_metadata.json');
+		const manifest = JSON.parse(fs.readFileSync(manifest_path, 'utf-8'));
+
+		expect(manifest).toHaveProperty('optimized.e2e-test-dep-layout-server');
+	});
+
+	test('optimizes +error.svelte dependencies', async ({ page }) => {
+		await page.goto('/');
+		await page.getByText('hello world!').waitFor();
+
+		const manifest_path = path.join(__dirname, '../node_modules/.vite/deps/_metadata.json');
+		const manifest = JSON.parse(fs.readFileSync(manifest_path, 'utf-8'));
+
+		expect(manifest).toHaveProperty('optimized.e2e-test-dep-error');
+	});
+
+	test('optimizes hooks.client.js dependencies', async ({ page }) => {
+		await page.goto('/');
+		await page.getByText('hello world!').waitFor();
+
+		const manifest_path = path.join(__dirname, '../node_modules/.vite/deps/_metadata.json');
+		const manifest = JSON.parse(fs.readFileSync(manifest_path, 'utf-8'));
+
+		expect(manifest).toHaveProperty('optimized.e2e-test-dep-hooks-client');
+	});
+
+	test('optimizes hooks.js dependencies', async ({ page }) => {
+		await page.goto('/');
+		await page.getByText('hello world!').waitFor();
+
+		const manifest_path = path.join(__dirname, '../node_modules/.vite/deps/_metadata.json');
+		const manifest = JSON.parse(fs.readFileSync(manifest_path, 'utf-8'));
+
+		expect(manifest).toHaveProperty('optimized.e2e-test-dep-hooks');
+	});
+});
diff --git a/packages/kit/test/apps/dev-only/vite.config.js b/packages/kit/test/apps/dev-only/vite.config.js
@@ -10,7 +10,7 @@ const config = {
 	optimizeDeps: {
 		// for CI, we need to explicitly prebundle deps, since
 		// the reload confuses Playwright
-		include: ['cookie', 'marked']
+		include: ['cookie']
 	},
 	plugins: [sveltekit()],
 	server: {
diff --git a/packages/kit/test/utils.js b/packages/kit/test/utils.js
@@ -242,7 +242,7 @@ export const config = {
 	// generous timeouts on CI
 	timeout: process.env.CI ? 45000 : 15000,
 	webServer: {
-		command: process.env.DEV ? 'pnpm dev' : 'pnpm build && pnpm preview',
+		command: process.env.DEV ? 'pnpm dev --force' : 'pnpm build && pnpm preview',
 		port: process.env.DEV ? 5173 : 4173
 	},
 	retries: process.env.CI ? 2 : 0,
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +"@sveltejs/kit": patch
 +---
++
 +fix: prevent excessive Vite dependency optimizations on navigation
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+module.exports = {`
	`2`	`+ cjs: () => 'cjs'`
	`3`	`+};`