Include styles for dynamically imported components (#5138)

* add failing test for #5137 * partial fix for #5137 * defer gathering of styles * fix typechecking * try and nudge turbo into not breaking everything * include dynamically imported styles during dev SSR * changeset * rename some stuff to make it clearer * simplify, and dont follow dynamic imports from entry * tidy up * tidy up * tidy up
sveltejs · Jul 5, 2022 · 4842a24 · 4842a24
1 parent d63bd72
commit 4842a24
Show file tree

Hide file tree

Showing 13 changed files with 165 additions and 95 deletions.
diff --git a/.changeset/wild-donuts-double.md b/.changeset/wild-donuts-double.md
@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+Include dynamically imported styles during SSR
diff --git a/packages/kit/src/runtime/server/page/load_node.js b/packages/kit/src/runtime/server/page/load_node.js
@@ -362,7 +362,7 @@ export async function load_node({
 
 		if (!loaded) {
 			// TODO do we still want to enforce this now that there's no fallthrough?
-			throw new Error(`load function must return a value${options.dev ? ` (${node.entry})` : ''}`);
+			throw new Error(`load function must return a value${options.dev ? ` (${node.file})` : ''}`);
 		}
 	} else if (shadow.body) {
 		loaded = {

diff --git a/packages/kit/src/runtime/server/page/render.js b/packages/kit/src/runtime/server/page/render.js
@@ -51,10 +51,14 @@ export async function render_response({
 		}
 	}
 
-	const stylesheets = new Set(options.manifest._.entry.css);
-	const modulepreloads = new Set(options.manifest._.entry.js);
+	const { entry } = options.manifest._;
+
+	const stylesheets = new Set(entry.stylesheets);
+	const modulepreloads = new Set(entry.imports);
+
 	/** @type {Map<string, string>} */
-	const styles = new Map();
+	// TODO if we add a client entry point one day, we will need to include inline_styles with the entry, otherwise stylesheets will be linked even if they are below inlineStyleThreshold
+	const inline_styles = new Map();
 
 	/** @type {Array<import('./types').Fetched>} */
 	const serialized_data = [];
@@ -72,18 +76,26 @@ export async function render_response({
 	}
 
 	if (resolve_opts.ssr) {
-		branch.forEach(({ node, props, loaded, fetched, uses_credentials }) => {
-			if (node.css) node.css.forEach((url) => stylesheets.add(url));
-			if (node.js) node.js.forEach((url) => modulepreloads.add(url));
-			if (node.styles) Object.entries(node.styles).forEach(([k, v]) => styles.set(k, v));
+		for (const { node, props, loaded, fetched, uses_credentials } of branch) {
+			if (node.imports) {
+				node.imports.forEach((url) => modulepreloads.add(url));
+			}
+
+			if (node.stylesheets) {
+				node.stylesheets.forEach((url) => stylesheets.add(url));
+			}
+
+			if (node.inline_styles) {
+				Object.entries(await node.inline_styles()).forEach(([k, v]) => inline_styles.set(k, v));
+			}
 
 			// TODO probably better if `fetched` wasn't populated unless `hydrate`
 			if (fetched && page_config.hydrate) serialized_data.push(...fetched);
 			if (props) shadow_props = props;
 
 			cache = loaded?.cache;
 			is_private = cache?.private ?? uses_credentials;
-		});
+		}
 
 		const session = writable($session);
 
@@ -144,8 +156,6 @@ export async function render_response({
 
 	let { head, html: body } = rendered;
 
-	const inlined_style = Array.from(styles.values()).join('\n');
-
 	await csp_ready;
 	const csp = new Csp(options.csp, {
 		dev: options.dev,
@@ -157,7 +167,7 @@ export async function render_response({
 
 	// prettier-ignore
 	const init_app = `
-		import { start } from ${s(options.prefix + options.manifest._.entry.file)};
+		import { start } from ${s(options.prefix + entry.file)};
 		start({
 			target: document.querySelector('[data-sveltekit-hydrate="${target}"]').parentNode,
 			paths: ${s(options.paths)},
@@ -185,14 +195,16 @@ export async function render_response({
 		}
 	`;
 
-	if (inlined_style) {
+	if (inline_styles.size > 0) {
+		const content = Array.from(inline_styles.values()).join('\n');
+
 		const attributes = [];
 		if (options.dev) attributes.push(' data-sveltekit');
 		if (csp.style_needs_nonce) attributes.push(` nonce="${csp.nonce}"`);
 
-		csp.add_style(inlined_style);
+		csp.add_style(content);
 
-		head += `\n\t<style${attributes.join('')}>${inlined_style}</style>`;
+		head += `\n\t<style${attributes.join('')}>${content}</style>`;
 	}
 
 	// prettier-ignore
@@ -207,7 +219,7 @@ export async function render_response({
 				attributes.push(`nonce="${csp.nonce}"`);
 			}
 
-			if (styles.has(dep)) {
+			if (inline_styles.has(dep)) {
 				// don't load stylesheets that are already inlined
 				// include them in disabled state so that Vite can detect them and doesn't try to add them
 				attributes.push('disabled', 'media="(max-width: 0)"');

diff --git a/packages/kit/src/vite/build/build_server.js b/packages/kit/src/vite/build/build_server.js
@@ -208,26 +208,22 @@ export async function build_server(options, client) {
 	});
 
 	manifest_data.components.forEach((component, i) => {
-		const file = `${output_dir}/server/nodes/${i}.js`;
-
-		const js = new Set();
-		const css = new Set();
-		find_deps(component, client.vite_manifest, js, css);
+		const entry = find_deps(client.vite_manifest, component, true);
 
 		const imports = [`import * as module from '../${vite_manifest[component].file}';`];
 
 		const exports = [
 			'export { module };',
 			`export const index = ${i};`,
-			`export const entry = '${client.vite_manifest[component].file}';`,
-			`export const js = ${s(Array.from(js))};`,
-			`export const css = ${s(Array.from(css))};`
+			`export const file = '${entry.file}';`,
+			`export const imports = ${s(entry.imports)};`,
+			`export const stylesheets = ${s(entry.stylesheets)};`
 		];
 
 		/** @type {string[]} */
 		const styles = [];
 
-		css.forEach((file) => {
+		entry.stylesheets.forEach((file) => {
 			if (stylesheet_lookup.has(file)) {
 				const index = stylesheet_lookup.get(file);
 				const name = `stylesheet_${index}`;
@@ -237,10 +233,11 @@ export async function build_server(options, client) {
 		});
 
 		if (styles.length > 0) {
-			exports.push(`export const styles = {\n${styles.join(',\n')}\n};`);
+			exports.push(`export const inline_styles = () => ({\n${styles.join(',\n')}\n});`);
 		}
 
-		fs.writeFileSync(file, `${imports.join('\n')}\n\n${exports.join('\n')}\n`);
+		const out = `${output_dir}/server/nodes/${i}.js`;
+		fs.writeFileSync(out, `${imports.join('\n')}\n\n${exports.join('\n')}\n`);
 	});
 
 	return {

diff --git a/packages/kit/src/vite/build/utils.js b/packages/kit/src/vite/build/utils.js
@@ -30,24 +30,47 @@ export async function create_build(config) {
 
 /**
  * Adds transitive JS and CSS dependencies to the js and css inputs.
- * @param {string} file
  * @param {import('vite').Manifest} manifest
- * @param {Set<string>} css
- * @param {Set<string>} js
+ * @param {string} entry
+ * @param {boolean} add_dynamic_css
  */
-export function find_deps(file, manifest, js, css) {
-	const chunk = manifest[file];
+export function find_deps(manifest, entry, add_dynamic_css) {
+	/** @type {Set<string>} */
+	const imports = new Set();
 
-	if (js.has(chunk.file)) return;
-	js.add(chunk.file);
+	/** @type {Set<string>} */
+	const stylesheets = new Set();
 
-	if (chunk.css) {
-		chunk.css.forEach((file) => css.add(file));
-	}
+	/**
+	 * @param {string} file
+	 * @param {boolean} add_js
+	 */
+	function traverse(file, add_js) {
+		const chunk = manifest[file];
+
+		if (imports.has(chunk.file)) return;
+		if (add_js) imports.add(chunk.file);
+
+		if (chunk.css) {
+			chunk.css.forEach((file) => stylesheets.add(file));
+		}
+
+		if (chunk.imports) {
+			chunk.imports.forEach((file) => traverse(file, add_js));
+		}
 
-	if (chunk.imports) {
-		chunk.imports.forEach((file) => find_deps(file, manifest, js, css));
+		if (add_dynamic_css && chunk.dynamicImports) {
+			chunk.dynamicImports.forEach((file) => traverse(file, false));
+		}
 	}
+
+	traverse(entry, true);
+
+	return {
+		file: manifest[entry].file,
+		imports: Array.from(imports),
+		stylesheets: Array.from(stylesheets)
+	};
 }
 
 /**

diff --git a/packages/kit/src/vite/dev/index.js b/packages/kit/src/vite/dev/index.js
@@ -50,8 +50,8 @@ export async function dev(vite, svelte_config) {
 			_: {
 				entry: {
 					file: `/@fs${runtime}/client/start.js`,
-					css: [],
-					js: []
+					imports: [],
+					stylesheets: []
 				},
 				nodes: manifest_data.components.map((id, index) => {
 					return async () => {
@@ -60,43 +60,46 @@ export async function dev(vite, svelte_config) {
 						const module = /** @type {import('types').SSRComponent} */ (
 							await vite.ssrLoadModule(url, { fixStacktrace: false })
 						);
-						const node = await vite.moduleGraph.getModuleByUrl(url);
-
-						if (!node) throw new Error(`Could not find node for ${url}`);
-
-						const deps = new Set();
-						await find_deps(vite, node, deps);
-
-						/** @type {Record<string, string>} */
-						const styles = {};
-
-						for (const dep of deps) {
-							const parsed = new URL(dep.url, 'http://localhost/');
-							const query = parsed.searchParams;
-
-							if (
-								style_pattern.test(dep.file) ||
-								(query.has('svelte') && query.get('type') === 'style')
-							) {
-								try {
-									const mod = await vite.ssrLoadModule(dep.url, { fixStacktrace: false });
-									styles[dep.url] = mod.default;
-								} catch {
-									// this can happen with dynamically imported modules, I think
-									// because the Vite module graph doesn't distinguish between
-									// static and dynamic imports? TODO investigate, submit fix
-								}
-							}
-						}
 
 						return {
 							module,
 							index,
-							entry: url.endsWith('.svelte') ? url : url + '?import',
-							css: [],
-							js: [],
+							file: url.endsWith('.svelte') ? url : url + '?import',
+							imports: [],
+							stylesheets: [],
 							// in dev we inline all styles to avoid FOUC
-							styles
+							inline_styles: async () => {
+								const node = await vite.moduleGraph.getModuleByUrl(url);
+
+								if (!node) throw new Error(`Could not find node for ${url}`);
+
+								const deps = new Set();
+								await find_deps(vite, node, deps);
+
+								/** @type {Record<string, string>} */
+								const styles = {};
+
+								for (const dep of deps) {
+									const parsed = new URL(dep.url, 'http://localhost/');
+									const query = parsed.searchParams;
+
+									if (
+										style_pattern.test(dep.file) ||
+										(query.has('svelte') && query.get('type') === 'style')
+									) {
+										try {
+											const mod = await vite.ssrLoadModule(dep.url, { fixStacktrace: false });
+											styles[dep.url] = mod.default;
+										} catch {
+											// this can happen with dynamically imported modules, I think
+											// because the Vite module graph doesn't distinguish between
+											// static and dynamic imports? TODO investigate, submit fix
+										}
+									}
+								}
+
+								return styles;
+							}
 						};
 					};
 				}),
@@ -430,6 +433,10 @@ async function find_deps(vite, node, deps) {
 		if (node.ssrTransformResult.deps) {
 			node.ssrTransformResult.deps.forEach((url) => branches.push(add_by_url(url)));
 		}
+
+		if (node.ssrTransformResult.dynamicDeps) {
+			node.ssrTransformResult.dynamicDeps.forEach((url) => branches.push(add_by_url(url)));
+		}
 	} else {
 		node.importedModules.forEach((node) => branches.push(add(node)));
 	}

diff --git a/packages/kit/src/vite/index.js b/packages/kit/src/vite/index.js
@@ -195,6 +195,11 @@ function kit() {
 			const verbose = vite_config.logLevel === 'info';
 			const log = logger({ verbose });
 
+			fs.writeFileSync(
+				`${paths.client_out_dir}/version.json`,
+				JSON.stringify({ version: process.env.VITE_SVELTEKIT_APP_VERSION })
+			);
+
 			/** @type {import('rollup').OutputChunk[]} */
 			const chunks = [];
 			/** @type {import('rollup').OutputAsset[]} */
@@ -213,25 +218,14 @@ function kit() {
 				fs.readFileSync(`${paths.client_out_dir}/immutable/manifest.json`, 'utf-8')
 			);
 
-			const entry = posixify(
+			const entry_id = posixify(
 				path.relative(cwd, `${get_runtime_path(svelte_config.kit)}/client/start.js`)
 			);
-			const entry_js = new Set();
-			const entry_css = new Set();
-			find_deps(entry, vite_manifest, entry_js, entry_css);
 
-			fs.writeFileSync(
-				`${paths.client_out_dir}/version.json`,
-				JSON.stringify({ version: process.env.VITE_SVELTEKIT_APP_VERSION })
-			);
 			const client = {
 				assets,
 				chunks,
-				entry: {
-					file: vite_manifest[entry].file,
-					js: Array.from(entry_js),
-					css: Array.from(entry_css)
-				},
+				entry: find_deps(vite_manifest, entry_id, false),
 				vite_manifest
 			};
 			log.info(`Client build completed. Wrote ${chunks.length} chunks and ${assets.length} assets`);

diff --git a/packages/kit/test/apps/basics/src/routes/load/dynamic-import-styles/_/Thing.svelte b/packages/kit/test/apps/basics/src/routes/load/dynamic-import-styles/_/Thing.svelte
@@ -0,0 +1,7 @@
+<p id="thing">this text is red</p>
+
+<style>
+	p {
+		color: red;
+	}
+</style>
diff --git a/packages/kit/test/apps/basics/src/routes/load/dynamic-import-styles/index.svelte b/packages/kit/test/apps/basics/src/routes/load/dynamic-import-styles/index.svelte
@@ -0,0 +1,15 @@
+<script context="module">
+	export async function load() {
+		return {
+			props: {
+				Thing: (await import('./_/Thing.svelte')).default
+			}
+		};
+	}
+</script>
+
+<script>
+	export let Thing;
+</script>
+
+<Thing />
diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js
@@ -1523,6 +1523,15 @@ test.describe('Load', () => {
 			);
 		}
 	});
+
+	test('CSS for dynamically imported components is reflected in server render', async ({
+		page
+	}) => {
+		await page.goto('/load/dynamic-import-styles');
+		expect(
+			await page.evaluate(() => getComputedStyle(document.querySelector('#thing')).color)
+		).toBe('rgb(255, 0, 0)');
+	});
 });
 
 test.describe('Method overrides', () => {