Move getStaticPaths call to happen during generation (#4047)

* Move getStaticPaths call to happen during generation * Adds a changeset * Update routing-priority.test.js * revert test change, clarify test purpose * Keep track of paths that have already been built Co-authored-by: Fred K. Schott <fkschott@gmail.com>
withastro · Jul 26, 2022 · 453c026 · 453c026
1 parent adf8cde
commit 453c026
Show file tree

Hide file tree

Showing 10 changed files with 143 additions and 88 deletions.
diff --git a/.changeset/old-stingrays-smoke.md b/.changeset/old-stingrays-smoke.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Perf: move getStaticPaths call during the build to during page generation
diff --git a/packages/astro/src/core/build/generate.ts b/packages/astro/src/core/build/generate.ts
@@ -9,8 +9,9 @@ import type {
 	EndpointHandler,
 	SSRLoadedRenderer,
 } from '../../@types/astro';
+import * as colors from 'kleur/colors';
 import type { BuildInternals } from '../../core/build/internal.js';
-import { joinPaths, prependForwardSlash, removeLeadingForwardSlash } from '../../core/path.js';
+import { joinPaths, prependForwardSlash, removeLeadingForwardSlash, removeTrailingForwardSlash } from '../../core/path.js';
 import type { RenderOptions } from '../../core/render/core';
 import { BEFORE_HYDRATION_SCRIPT_ID } from '../../vite-plugin-scripts/index.js';
 import { call as callEndpoint } from '../endpoint/index.js';
@@ -23,6 +24,8 @@ import { getOutFile, getOutFolder } from './common.js';
 import { eachPageData, getPageDataByComponent } from './internal.js';
 import type { PageBuildData, SingleFileBuiltModule, StaticBuildOptions } from './types';
 import { getTimeStat } from './util.js';
+import { callGetStaticPaths, RouteCache, RouteCacheEntry } from '../render/route-cache.js';
+import { matchRoute } from '../routing/match.js';
 
 // Render is usually compute, which Node.js can't parallelize well.
 // In real world testing, dropping from 10->1 showed a notiable perf
@@ -89,10 +92,8 @@ export function chunkIsPage(
 }
 
 export async function generatePages(
-	result: RollupOutput,
 	opts: StaticBuildOptions,
 	internals: BuildInternals,
-	facadeIdToPageDataMap: Map<string, PageBuildData>
 ) {
 	const timer = performance.now();
 	info(opts.logging, null, `\n${bgGreen(black(' generating static routes '))}`);
@@ -102,19 +103,20 @@ export async function generatePages(
 	const outFolder = ssr ? opts.buildConfig.server : opts.astroConfig.outDir;
 	const ssrEntryURL = new URL('./' + serverEntry + `?time=${Date.now()}`, outFolder);
 	const ssrEntry = await import(ssrEntryURL.toString());
+	const builtPaths = new Set<string>;
 
 	for (const pageData of eachPageData(internals)) {
-		await generatePage(opts, internals, pageData, ssrEntry);
+		await generatePage(opts, internals, pageData, ssrEntry, builtPaths);
 	}
 	info(opts.logging, null, dim(`Completed in ${getTimeStat(timer, performance.now())}.\n`));
 }
 
 async function generatePage(
-	//output: OutputChunk,
 	opts: StaticBuildOptions,
 	internals: BuildInternals,
 	pageData: PageBuildData,
-	ssrEntry: SingleFileBuiltModule
+	ssrEntry: SingleFileBuiltModule,
+	builtPaths: Set<string>,
 ) {
 	let timeStart = performance.now();
 	const renderers = ssrEntry.renderers;
@@ -148,18 +150,89 @@ async function generatePage(
 	const icon = pageData.route.type === 'page' ? green('▶') : magenta('λ');
 	info(opts.logging, null, `${icon} ${pageData.route.component}`);
 
-	for (let i = 0; i < pageData.paths.length; i++) {
-		const path = pageData.paths[i];
+	// Get paths for the route, calling getStaticPaths if needed.
+	const paths = await getPathsForRoute(pageData, pageModule, opts, builtPaths);
+
+	for (let i = 0; i < paths.length; i++) {
+		const path = paths[i];
 		await generatePath(path, opts, generationOptions);
 		const timeEnd = performance.now();
 		const timeChange = getTimeStat(timeStart, timeEnd);
 		const timeIncrease = `(+${timeChange})`;
 		const filePath = getOutputFilename(opts.astroConfig, path);
-		const lineIcon = i === pageData.paths.length - 1 ? '└─' : '├─';
+		const lineIcon = i === paths.length - 1 ? '└─' : '├─';
 		info(opts.logging, null, `  ${cyan(lineIcon)} ${dim(filePath)} ${dim(timeIncrease)}`);
 	}
 }
 
+async function getPathsForRoute(
+	pageData: PageBuildData,
+	mod: ComponentInstance,
+	opts: StaticBuildOptions,
+	builtPaths: Set<string>,
+): Promise<Array<string>> {
+	let paths: Array<string> = [];
+	if(pageData.route.pathname) {
+		paths.push(pageData.route.pathname);
+		builtPaths.add(pageData.route.pathname);
+	} else {
+		const route = pageData.route;
+		const result = await callGetStaticPaths({
+			mod,
+			route: pageData.route,
+			isValidate: false,
+			logging: opts.logging,
+			ssr: opts.astroConfig.output === 'server'
+		})
+		.then((_result) => {
+			const label = _result.staticPaths.length === 1 ? 'page' : 'pages';
+			debug(
+				'build',
+				`├── ${colors.bold(colors.green('✔'))} ${route.component} → ${colors.magenta(
+					`[${_result.staticPaths.length} ${label}]`
+				)}`
+			);
+			return _result;
+		})
+		.catch((err) => {
+			debug('build', `├── ${colors.bold(colors.red('✗'))} ${route.component}`);
+			throw err;
+		});
+
+		// Save the route cache so it doesn't get called again
+		opts.routeCache.set(route, result);
+
+		paths = result.staticPaths
+			.map((staticPath) => staticPath.params && route.generate(staticPath.params))
+			.filter((staticPath) => {
+				// Remove empty or undefined paths
+				if (!staticPath) {
+					return false;
+				}
+
+				// The path hasn't been built yet, include it
+				if (!builtPaths.has(removeTrailingForwardSlash(staticPath))) {
+					return true;
+				}
+
+				// The path was already built once. Check the manifest to see if
+				// this route takes priority for the final URL.
+				// NOTE: The same URL may match multiple routes in the manifest.
+				// Routing priority needs to be verified here for any duplicate
+				// paths to ensure routing priority rules are enforced in the final build.
+				const matchedRoute = matchRoute(staticPath, opts.manifest);
+				return matchedRoute === route;
+			});
+
+		// Add each path to the builtPaths set, to avoid building it again later.
+		for(const staticPath of paths) {
+			builtPaths.add(removeTrailingForwardSlash(staticPath));
+		}
+	}
+
+	return paths;
+}
+
 interface GeneratePathOptions {
 	pageData: PageBuildData;
 	internals: BuildInternals;

diff --git a/packages/astro/src/core/build/page-data.ts b/packages/astro/src/core/build/page-data.ts
@@ -1,15 +1,12 @@
 import type { ViteDevServer } from 'vite';
-import type { AstroConfig, ComponentInstance, ManifestData, RouteData } from '../../@types/astro';
+import type { AstroConfig, ManifestData } from '../../@types/astro';
 import type { LogOptions } from '../logger/core';
 import { info } from '../logger/core.js';
 import type { AllPagesData } from './types';
 
 import * as colors from 'kleur/colors';
-import { fileURLToPath } from 'url';
 import { debug } from '../logger/core.js';
-import { removeTrailingForwardSlash } from '../path.js';
-import { callGetStaticPaths, RouteCache, RouteCacheEntry } from '../render/route-cache.js';
-import { matchRoute } from '../routing/match.js';
+import { RouteCache } from '../render/route-cache.js';
 
 export interface CollectPagesDataOptions {
 	astroConfig: AstroConfig;
@@ -30,7 +27,7 @@ export interface CollectPagesDataResult {
 export async function collectPagesData(
 	opts: CollectPagesDataOptions
 ): Promise<CollectPagesDataResult> {
-	const { astroConfig, logging, manifest, origin, routeCache, viteServer } = opts;
+	const { astroConfig,  manifest } = opts;
 
 	const assets: Record<string, string> = {};
 	const allPages: AllPagesData = {};
@@ -61,7 +58,6 @@ export async function collectPagesData(
 			allPages[route.component] = {
 				component: route.component,
 				route,
-				paths: [route.pathname],
 				moduleSpecifier: '',
 				css: new Set(),
 				hoistedScript: undefined,
@@ -80,49 +76,9 @@ export async function collectPagesData(
 			continue;
 		}
 		// dynamic route:
-		const result = await getStaticPathsForRoute(opts, route)
-			.then((_result) => {
-				const label = _result.staticPaths.length === 1 ? 'page' : 'pages';
-				debug(
-					'build',
-					`├── ${colors.bold(colors.green('✔'))} ${route.component} → ${colors.magenta(
-						`[${_result.staticPaths.length} ${label}]`
-					)}`
-				);
-				return _result;
-			})
-			.catch((err) => {
-				debug('build', `├── ${colors.bold(colors.red('✗'))} ${route.component}`);
-				throw err;
-			});
-		const finalPaths = result.staticPaths
-			.map((staticPath) => staticPath.params && route.generate(staticPath.params))
-			.filter((staticPath) => {
-				// Remove empty or undefined paths
-				if (!staticPath) {
-					return false;
-				}
-
-				// The path hasn't been built yet, include it
-				if (!builtPaths.has(removeTrailingForwardSlash(staticPath))) {
-					return true;
-				}
-
-				// The path was already built once. Check the manifest to see if
-				// this route takes priority for the final URL.
-				// NOTE: The same URL may match multiple routes in the manifest.
-				// Routing priority needs to be verified here for any duplicate
-				// paths to ensure routing priority rules are enforced in the final build.
-				const matchedRoute = matchRoute(staticPath, manifest);
-				return matchedRoute === route;
-			});
-
-		finalPaths.map((staticPath) => builtPaths.add(removeTrailingForwardSlash(staticPath)));
-
 		allPages[route.component] = {
 			component: route.component,
 			route,
-			paths: finalPaths,
 			moduleSpecifier: '',
 			css: new Set(),
 			hoistedScript: undefined,
@@ -133,16 +89,3 @@ export async function collectPagesData(
 
 	return { assets, allPages };
 }
-
-async function getStaticPathsForRoute(
-	opts: CollectPagesDataOptions,
-	route: RouteData
-): Promise<RouteCacheEntry> {
-	const { astroConfig, logging, routeCache, ssr, viteServer } = opts;
-	if (!viteServer) throw new Error(`vite.createServer() not called!`);
-	const filePath = new URL(`./${route.component}`, astroConfig.root);
-	const mod = (await viteServer.ssrLoadModule(fileURLToPath(filePath))) as ComponentInstance;
-	const result = await callGetStaticPaths({ mod, route, isValidate: false, logging, ssr });
-	routeCache.set(route, result);
-	return result;
-}
diff --git a/packages/astro/src/core/build/static-build.ts b/packages/astro/src/core/build/static-build.ts
@@ -1,7 +1,6 @@
 import glob from 'fast-glob';
 import fs from 'fs';
 import { bgGreen, bgMagenta, black, dim } from 'kleur/colors';
-import type { RollupOutput } from 'rollup';
 import { fileURLToPath } from 'url';
 import * as vite from 'vite';
 import { BuildInternals, createBuildInternals } from '../../core/build/internal.js';
@@ -72,7 +71,7 @@ Example:
 	// Build your project (SSR application code, assets, client JS, etc.)
 	timer.ssr = performance.now();
 	info(opts.logging, 'build', `Building ${astroConfig.output} entrypoints...`);
-	const ssrResult = (await ssrBuild(opts, internals, pageInput)) as RollupOutput;
+	await ssrBuild(opts, internals, pageInput);
 	info(opts.logging, 'build', dim(`Completed in ${getTimeStat(timer.ssr, performance.now())}.`));
 
 	const rendererClientEntrypoints = opts.astroConfig._ctx.renderers
@@ -93,7 +92,7 @@ Example:
 	timer.generate = performance.now();
 	if (astroConfig.output === 'static') {
 		try {
-			await generatePages(ssrResult, opts, internals, facadeIdToPageDataMap);
+			await generatePages(opts, internals);
 		} finally {
 			await cleanSsrOutput(opts);
 		}

diff --git a/packages/astro/src/core/build/types.ts b/packages/astro/src/core/build/types.ts
@@ -16,7 +16,6 @@ export type ViteID = string;
 
 export interface PageBuildData {
 	component: ComponentPath;
-	paths: string[];
 	route: RouteData;
 	moduleSpecifier: string;
 	css: Set<string>;

diff --git a/packages/astro/test/astro-get-static-paths.test.js b/packages/astro/test/astro-get-static-paths.test.js
@@ -52,14 +52,17 @@ describe('getStaticPaths - 404 behavior', () => {
 });
 
 describe('getStaticPaths - route params type validation', () => {
-	let fixture;
-	let devServer;
+	let fixture, devServer;
 
 	before(async () => {
 		fixture = await loadFixture({ root: './fixtures/astro-get-static-paths/' });
 		devServer = await fixture.startDevServer();
 	});
 
+	after(async () => {
+		await devServer.stop();
+	});
+
 	it('resolves 200 on matching static path - string params', async () => {
 		// route provided with { params: { year: "2022", slug: "post-2" }}
 		const res = await fixture.fetch('/blog/2022/post-1');

diff --git a/packages/astro/test/fixtures/lit-element/src/pages/[page].astro b/packages/astro/test/fixtures/lit-element/src/pages/[page].astro
@@ -0,0 +1,30 @@
+---
+import {MyElement} from '../components/my-element.js';
+export async function getStaticPaths() {
+	return [
+		{
+			params: { page: 1 },
+		},
+		{
+			params: { page: 2 },
+		},
+		{
+			params: { page: 3 }
+		}
+	]
+};
+
+const { page } = Astro.params
+---
+
+<html lang="en">
+	<head>
+		<meta charset="utf-8" />
+		<meta name="viewport" content="width=device-width" />
+		<title>Posts Page {page}</title>
+	</head>
+	<body>
+		<h1>Welcome to page {page}</h1>
+		<MyElement />
+	</body>
+</html>
diff --git a/packages/astro/test/fixtures/routing-priority/src/pages/de/index.astro b/packages/astro/test/fixtures/routing-priority/src/pages/de/index.astro
@@ -1,4 +1,5 @@
 ---
+// This route should take priority over src/pages/[lang]/index.astro
 ---
 <html lang="en">
 	<head>
@@ -7,6 +8,7 @@
 		<title>Routing</title>
 	</head>
 	<body>
-		<h1>de/index.astro</h1>
-	</body>
+		<h1>de/index.astro (priority)</h1>
+		<p>de</p>
+</body>
 </html>
diff --git a/packages/astro/test/lit-element.test.js b/packages/astro/test/lit-element.test.js
@@ -80,4 +80,9 @@ describe('LitElement test', function () {
 		// has named slot content in lightdom
 		expect($(namedSlot).text()).to.equal('named');
 	});
+
+	it('Is able to build when behind getStaticPaths', async () => {
+		const dynamicPage = await fixture.readFile('/1/index.html');
+		expect(dynamicPage.length).to.be.greaterThan(0);
+	});
 });