Improve content collection styles and scripts build perf (#10959)

* Improve content collection styles and scripts build perf * Update test It was actually a bug. There was an empty module script injected. * Skip test * Fix test not matching non-ccc behaviour * Workaround bug to make test pass * Update .changeset/grumpy-pillows-develop.md Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca> --------- Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
withastro · May 8, 2024 · 685fc22 · 685fc22
1 parent cceeafb
commit 685fc22
Show file tree

Hide file tree

Showing 19 changed files with 287 additions and 219 deletions.
diff --git a/.changeset/grumpy-pillows-develop.md b/.changeset/grumpy-pillows-develop.md
@@ -0,0 +1,5 @@
+---
+"astro": patch
+---
+
+Refactors internal handling of styles and scripts for content collections to improve build performance
diff --git a/packages/astro/src/content/vite-plugin-content-assets.ts b/packages/astro/src/content/vite-plugin-content-assets.ts
@@ -3,8 +3,7 @@ import { pathToFileURL } from 'node:url';
 import type { Plugin, Rollup } from 'vite';
 import type { AstroSettings, SSRElement } from '../@types/astro.js';
 import { getAssetsPrefix } from '../assets/utils/getAssetsPrefix.js';
-import { getParentModuleInfos, moduleIsTopLevelPage } from '../core/build/graph.js';
-import { type BuildInternals, getPageDataByViteID } from '../core/build/internal.js';
+import type { BuildInternals } from '../core/build/internal.js';
 import type { AstroBuildPlugin } from '../core/build/plugin.js';
 import type { StaticBuildOptions } from '../core/build/types.js';
 import type { ModuleLoader } from '../core/module-loader/loader.js';
@@ -163,49 +162,25 @@ export function astroConfigBuildPlugin(
 						chunk.type === 'chunk' &&
 						(chunk.code.includes(LINKS_PLACEHOLDER) || chunk.code.includes(SCRIPTS_PLACEHOLDER))
 					) {
-						let entryStyles = new Set<string>();
-						let entryLinks = new Set<string>();
-						let entryScripts = new Set<string>();
+						const entryStyles = new Set<string>();
+						const entryLinks = new Set<string>();
+						const entryScripts = new Set<string>();
 
-						if (options.settings.config.experimental.contentCollectionCache) {
-							// TODO: hoisted scripts are still handled on the pageData rather than the asset propagation point
-							for (const id of chunk.moduleIds) {
-								const _entryCss = internals.propagatedStylesMap.get(id);
-								const _entryScripts = internals.propagatedScriptsMap.get(id);
-								if (_entryCss) {
-									for (const value of _entryCss) {
-										if (value.type === 'inline') entryStyles.add(value.content);
-										if (value.type === 'external') entryLinks.add(value.src);
-									}
-								}
-								if (_entryScripts) {
-									for (const value of _entryScripts) {
-										entryScripts.add(value);
-									}
+						for (const id of chunk.moduleIds) {
+							const _entryCss = internals.propagatedStylesMap.get(id);
+							const _entryScripts = internals.propagatedScriptsMap.get(id);
+							if (_entryCss) {
+								// TODO: Separating styles and links this way is not ideal. The `entryCss` list is order-sensitive
+								// and splitting them into two sets causes the order to be lost, because styles are rendered after
+								// links. Refactor this away in the future.
+								for (const value of _entryCss) {
+									if (value.type === 'inline') entryStyles.add(value.content);
+									if (value.type === 'external') entryLinks.add(value.src);
 								}
 							}
-						} else {
-							for (const id of Object.keys(chunk.modules)) {
-								for (const pageInfo of getParentModuleInfos(id, ssrPluginContext!)) {
-									if (moduleIsTopLevelPage(pageInfo)) {
-										const pageViteID = pageInfo.id;
-										const pageData = getPageDataByViteID(internals, pageViteID);
-										if (!pageData) continue;
-
-										const _entryCss = pageData.propagatedStyles?.get(id);
-										const _entryScripts = pageData.propagatedScripts?.get(id);
-										if (_entryCss) {
-											for (const value of _entryCss) {
-												if (value.type === 'inline') entryStyles.add(value.content);
-												if (value.type === 'external') entryLinks.add(value.src);
-											}
-										}
-										if (_entryScripts) {
-											for (const value of _entryScripts) {
-												entryScripts.add(value);
-											}
-										}
-									}
+							if (_entryScripts) {
+								for (const value of _entryScripts) {
+									entryScripts.add(value);
 								}
 							}
 						}

diff --git a/packages/astro/src/core/build/internal.ts b/packages/astro/src/core/build/internal.ts
@@ -25,8 +25,6 @@ export interface BuildInternals {
 	hoistedScriptIdToHoistedMap: Map<string, Set<string>>;
 	// A mapping of hoisted script ids back to the pages which reference it
 	hoistedScriptIdToPagesMap: Map<string, Set<string>>;
-	// A mapping of hoisted script ids back to the content which reference it
-	hoistedScriptIdToContentMap: Map<string, Set<string>>;
 
 	/**
 	 * Used by the `directRenderScript` option. If script is inlined, its id and
@@ -93,7 +91,15 @@ export interface BuildInternals {
 	cachedClientEntries: string[];
 	cacheManifestUsed: boolean;
 
+	/**
+	 * Map of propagated module ids (usually something like `/Users/...blog.mdx?astroPropagatedAssets`)
+	 * to a set of stylesheets that it uses.
+	 */
 	propagatedStylesMap: Map<string, Set<StylesheetAsset>>;
+	/**
+	 * Map of propagated module ids (usually something like `/Users/...blog.mdx?astroPropagatedAssets`)
+	 * to a set of hoisted scripts that it uses.
+	 */
 	propagatedScriptsMap: Map<string, Set<string>>;
 
 	// A list of all static files created during the build. Used for SSR.
@@ -125,7 +131,6 @@ export function createBuildInternals(): BuildInternals {
 		cssModuleToChunkIdMap: new Map(),
 		hoistedScriptIdToHoistedMap,
 		hoistedScriptIdToPagesMap,
-		hoistedScriptIdToContentMap: new Map(),
 		inlinedScripts: new Map(),
 		entrySpecifierToBundleMap: new Map<string, string>(),
 		pageToBundleMap: new Map<string, string>(),

diff --git a/packages/astro/src/core/build/page-data.ts b/packages/astro/src/core/build/page-data.ts
@@ -53,8 +53,6 @@ export async function collectPagesData(
 				route,
 				moduleSpecifier: '',
 				styles: [],
-				propagatedStyles: new Map(),
-				propagatedScripts: new Map(),
 				hoistedScript: undefined,
 				hasSharedModules: false,
 			};
@@ -78,8 +76,6 @@ export async function collectPagesData(
 			route,
 			moduleSpecifier: '',
 			styles: [],
-			propagatedStyles: new Map(),
-			propagatedScripts: new Map(),
 			hoistedScript: undefined,
 			hasSharedModules: false,
 		};

diff --git a/packages/astro/src/core/build/plugins/plugin-analyzer.ts b/packages/astro/src/core/build/plugins/plugin-analyzer.ts
@@ -6,7 +6,6 @@ import type { AstroBuildPlugin } from '../plugin.js';
 
 import { PROPAGATED_ASSET_FLAG } from '../../../content/consts.js';
 import { prependForwardSlash } from '../../../core/path.js';
-import { isContentCollectionsCacheEnabled } from '../../../core/util.js';
 import {
 	getParentModuleInfos,
 	getTopLevelPageModuleInfos,
@@ -32,9 +31,7 @@ export function vitePluginAnalyzer(
 		const pageScripts = new Map<
 			string,
 			{
-				type: 'page' | 'content';
 				hoistedSet: Set<string>;
-				propagatedMapByImporter: Map<string, Set<string>>;
 			}
 		>();
 
@@ -53,48 +50,12 @@ export function vitePluginAnalyzer(
 				if (hoistedScripts.size) {
 					for (const parentInfo of getParentModuleInfos(from, this, isPropagatedAsset)) {
 						if (isPropagatedAsset(parentInfo.id)) {
-							if (isContentCollectionsCacheEnabled(options.settings.config)) {
-								if (!pageScripts.has(parentInfo.id)) {
-									pageScripts.set(parentInfo.id, {
-										type: 'content',
-										hoistedSet: new Set(),
-										propagatedMapByImporter: new Map(),
-									});
-								}
-								const propagaters = pageScripts.get(parentInfo.id)!.propagatedMapByImporter;
-								for (const hid of hoistedScripts) {
-									if (!propagaters.has(parentInfo.id)) {
-										propagaters.set(parentInfo.id, new Set());
-									}
-									propagaters.get(parentInfo.id)!.add(hid);
-								}
-							} else {
-								for (const nestedParentInfo of getParentModuleInfos(from, this)) {
-									if (moduleIsTopLevelPage(nestedParentInfo)) {
-										for (const hid of hoistedScripts) {
-											if (!pageScripts.has(nestedParentInfo.id)) {
-												pageScripts.set(nestedParentInfo.id, {
-													type: 'page',
-													hoistedSet: new Set(),
-													propagatedMapByImporter: new Map(),
-												});
-											}
-											const entry = pageScripts.get(nestedParentInfo.id)!;
-											if (!entry.propagatedMapByImporter.has(parentInfo.id)) {
-												entry.propagatedMapByImporter.set(parentInfo.id, new Set());
-											}
-											entry.propagatedMapByImporter.get(parentInfo.id)!.add(hid);
-										}
-									}
-								}
-							}
+							internals.propagatedScriptsMap.set(parentInfo.id, hoistedScripts);
 						} else if (moduleIsTopLevelPage(parentInfo)) {
 							for (const hid of hoistedScripts) {
 								if (!pageScripts.has(parentInfo.id)) {
 									pageScripts.set(parentInfo.id, {
-										type: 'page',
 										hoistedSet: new Set(),
-										propagatedMapByImporter: new Map(),
 									});
 								}
 								pageScripts.get(parentInfo.id)?.hoistedSet.add(hid);
@@ -105,21 +66,20 @@ export function vitePluginAnalyzer(
 			},
 
 			finalize() {
-				for (const [pageId, { hoistedSet, propagatedMapByImporter, type }] of pageScripts) {
-					let astroModuleId: string;
-					if (type === 'page') {
-						const pageData = getPageDataByViteID(internals, pageId);
-						if (!pageData) {
-							continue;
-						}
-						const { component } = pageData;
-						astroModuleId = prependForwardSlash(component);
-
-						// Keep track of the importers
-						pageData.propagatedScripts = propagatedMapByImporter;
-					} else {
-						astroModuleId = pageId;
+				// Add propagated scripts to client build,
+				// but DON'T add to pages -> hoisted script map.
+				for (const propagatedScripts of internals.propagatedScriptsMap.values()) {
+					for (const propagatedScript of propagatedScripts) {
+						internals.discoveredScripts.add(propagatedScript);
 					}
+				}
+
+				for (const [pageId, { hoistedSet }] of pageScripts) {
+					const pageData = getPageDataByViteID(internals, pageId);
+					if (!pageData) continue;
+
+					const { component } = pageData;
+					const astroModuleId = prependForwardSlash(component);
 
 					const uniqueHoistedId = JSON.stringify(Array.from(hoistedSet).sort());
 					let moduleId: string;
@@ -134,32 +94,13 @@ export function vitePluginAnalyzer(
 					}
 					internals.discoveredScripts.add(moduleId);
 
-					// Add propagated scripts to client build,
-					// but DON'T add to pages -> hoisted script map.
-					for (const propagatedScripts of propagatedMapByImporter.values()) {
-						for (const propagatedScript of propagatedScripts) {
-							internals.discoveredScripts.add(propagatedScript);
-						}
-					}
-
-					if (type === 'page') {
-						// Make sure to track that this page uses this set of hoisted scripts
-						if (internals.hoistedScriptIdToPagesMap.has(moduleId)) {
-							const pages = internals.hoistedScriptIdToPagesMap.get(moduleId);
-							pages!.add(astroModuleId);
-						} else {
-							internals.hoistedScriptIdToPagesMap.set(moduleId, new Set([astroModuleId]));
-							internals.hoistedScriptIdToHoistedMap.set(moduleId, hoistedSet);
-						}
+					// Make sure to track that this page uses this set of hoisted scripts
+					if (internals.hoistedScriptIdToPagesMap.has(moduleId)) {
+						const pages = internals.hoistedScriptIdToPagesMap.get(moduleId);
+						pages!.add(astroModuleId);
 					} else {
-						// For content collections save to hoistedScriptIdToContentMap instead
-						if (internals.hoistedScriptIdToContentMap.has(moduleId)) {
-							const contentModules = internals.hoistedScriptIdToContentMap.get(moduleId);
-							contentModules!.add(astroModuleId);
-						} else {
-							internals.hoistedScriptIdToContentMap.set(moduleId, new Set([astroModuleId]));
-							internals.hoistedScriptIdToHoistedMap.set(moduleId, hoistedSet);
-						}
+						internals.hoistedScriptIdToPagesMap.set(moduleId, new Set([astroModuleId]));
+						internals.hoistedScriptIdToHoistedMap.set(moduleId, hoistedSet);
 					}
 				}
 			},

diff --git a/packages/astro/src/core/build/plugins/plugin-content.ts b/packages/astro/src/core/build/plugins/plugin-content.ts
@@ -171,6 +171,7 @@ function vitePluginContent(
 		outputOptions(outputOptions) {
 			const rootPath = normalizePath(fileURLToPath(opts.settings.config.root));
 			const srcPath = normalizePath(fileURLToPath(opts.settings.config.srcDir));
+			const entryCache = new Map<string, string>();
 			extendManualChunks(outputOptions, {
 				before(id, meta) {
 					if (id.startsWith(srcPath) && id.slice(srcPath.length).startsWith('content')) {
@@ -186,7 +187,11 @@ function vitePluginContent(
 							return resultId;
 						}
 						const [srcRelativePath, flag] = id.replace(rootPath, '/').split('?');
-						const collectionEntry = findEntryFromSrcRelativePath(lookupMap, srcRelativePath);
+						const collectionEntry = findEntryFromSrcRelativePath(
+							lookupMap,
+							srcRelativePath,
+							entryCache
+						);
 						if (collectionEntry) {
 							let suffix = '.mjs';
 							if (flag === PROPAGATED_ASSET_FLAG) {
@@ -273,8 +278,11 @@ function vitePluginContent(
 	};
 }
 
-const entryCache = new Map<string, string>();
-function findEntryFromSrcRelativePath(lookupMap: ContentLookupMap, srcRelativePath: string) {
+function findEntryFromSrcRelativePath(
+	lookupMap: ContentLookupMap,
+	srcRelativePath: string,
+	entryCache: Map<string, string>
+) {
 	let value = entryCache.get(srcRelativePath);
 	if (value) return value;
 	for (const collection of Object.values(lookupMap)) {