Fixes for redirects config (#7644)

* Update redirects static generation based on recs Got some great recommendations on how to handle our HTML written redirect code based on SEO best practices. See withastro/roadmap#466 (comment) This implements them all. * Fix for using the root path / as a redirect Fixes #7478 * Fix static redirects prefer over dynamic page Fixes #7581
withastro · Jul 13, 2023 · 213e109 · 213e109
1 parent af5827d
commit 213e109
Show file tree

Hide file tree

Showing 9 changed files with 110 additions and 14 deletions.
diff --git a/.changeset/four-pumpkins-try.md b/.changeset/four-pumpkins-try.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fix for allowing the root path / as a redirect
diff --git a/.changeset/light-eggs-hug.md b/.changeset/light-eggs-hug.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fix static redirects prefered over dynamic regular routes
diff --git a/packages/astro/src/core/build/generate.ts b/packages/astro/src/core/build/generate.ts
@@ -619,9 +619,18 @@ async function generatePath(
 				return;
 			}
 			const location = getRedirectLocationOrThrow(response.headers);
+			const fromPath = new URL(renderContext.request.url).pathname;
+			// A short delay causes Google to interpret the redirect as temporary.
+			// https://developers.google.com/search/docs/crawling-indexing/301-redirects#metarefresh
+			const delay = response.status === 302 ? 2 : 0;
 			body = `<!doctype html>
 <title>Redirecting to: ${location}</title>
-<meta http-equiv="refresh" content="0;url=${location}" />`;
+<meta http-equiv="refresh" content="${delay};url=${location}">
+<meta name="robots" content="noindex">
+<link rel="canonical" href="${location}">
+<body>
+	<a href="${location}">Redirecting from <code>${fromPath}</code> to <code>${location}</code></a>
+</body>`;
 			// A dynamic redirect, set the location so that integrations know about it.
 			if (pageData.route.type !== 'redirect') {
 				pageData.route.redirect = location;

diff --git a/packages/astro/src/core/build/static-build.ts b/packages/astro/src/core/build/static-build.ts
@@ -32,6 +32,7 @@ import { RESOLVED_SPLIT_MODULE_ID, SSR_VIRTUAL_MODULE_ID } from './plugins/plugi
 import { ASTRO_PAGE_EXTENSION_POST_PATTERN } from './plugins/util.js';
 import type { PageBuildData, StaticBuildOptions } from './types';
 import { getTimeStat } from './util.js';
+import { routeIsRedirect } from '../redirects/index.js';
 
 export async function viteBuild(opts: StaticBuildOptions) {
 	const { allPages, settings } = opts;
@@ -60,8 +61,10 @@ export async function viteBuild(opts: StaticBuildOptions) {
 		// Track the page data in internals
 		trackPageData(internals, component, pageData, astroModuleId, astroModuleURL);
 
-		pageInput.add(astroModuleId);
-		facadeIdToPageDataMap.set(fileURLToPath(astroModuleURL), pageData);
+		if(!routeIsRedirect(pageData.route)) {
+			pageInput.add(astroModuleId);
+			facadeIdToPageDataMap.set(fileURLToPath(astroModuleURL), pageData);
+		}
 	}
 
 	// Empty out the dist folder, if needed. Vite has a config for doing this

diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts
@@ -458,7 +458,28 @@ export function createRouteManifest(
 			redirectRoute: routes.find((r) => r.route === to),
 		};
 
-		// Push so that redirects are selected last.
+		const lastSegmentIsDynamic = (r: RouteData) => !!r.segments.at(-1)?.at(-1)?.dynamic;
+
+		const redirBase = path.posix.dirname(route);
+		const dynamicRedir = lastSegmentIsDynamic(routeData);
+		let i = 0;
+		for(const existingRoute of routes) {
+			// An exact match, prefer the page/endpoint. This matches hosts.
+			if(existingRoute.route === route) {
+				routes.splice(i+1, 0, routeData);
+				return;
+			}
+
+			// If the existing route is dynamic, prefer the static redirect.
+			const base = path.posix.dirname(existingRoute.route);
+			if(base === redirBase && !dynamicRedir && lastSegmentIsDynamic(existingRoute)) {
+				routes.splice(i, 0, routeData);
+				return;
+			}
+			i++;
+		}
+
+		// Didn't find a good place, insert last
 		routes.push(routeData);
 	});
 

diff --git a/packages/astro/test/fixtures/ssr-redirect/src/middleware.ts b/packages/astro/test/fixtures/ssr-redirect/src/middleware.ts
@@ -5,7 +5,7 @@ export const onRequest = defineMiddleware(({ request }, next) => {
 		return new Response(null, {
 			status: 301,
 			headers: {
-				'Location': '/'
+				'Location': '/test'
 			}
 		});
 	}

diff --git a/...xtures/ssr-redirect/src/pages/index.astro → ...ixtures/ssr-redirect/src/pages/test.astro b/...xtures/ssr-redirect/src/pages/index.astro → ...ixtures/ssr-redirect/src/pages/test.astro
diff --git a/packages/astro/test/redirects.test.js b/packages/astro/test/redirects.test.js
@@ -13,7 +13,7 @@ describe('Astro.redirect', () => {
 				output: 'server',
 				adapter: testAdapter(),
 				redirects: {
-					'/api/redirect': '/',
+					'/api/redirect': '/test',
 				},
 				experimental: {
 					redirects: true,
@@ -75,12 +75,13 @@ describe('Astro.redirect', () => {
 						redirects: true,
 					},
 					redirects: {
-						'/one': '/',
-						'/two': '/',
+						'/': '/test',
+						'/one': '/test',
+						'/two': '/test',
 						'/blog/[...slug]': '/articles/[...slug]',
 						'/three': {
 							status: 302,
-							destination: '/',
+							destination: '/test',
 						},
 					},
 				});
@@ -93,18 +94,44 @@ describe('Astro.redirect', () => {
 				expect(html).to.include('url=/login');
 			});
 
+			it('Includes the meta noindex tag', async () => {
+				const html = await fixture.readFile('/secret/index.html');
+				expect(html).to.include('name="robots');
+				expect(html).to.include('content="noindex');
+			});
+
+			it('Includes a link to the new pages for bots to follow', async () => {
+				const html = await fixture.readFile('/secret/index.html');
+				expect(html).to.include('<a href="/login">');
+			});
+
+			it('Includes a canonical link', async () => {
+				const html = await fixture.readFile('/secret/index.html');
+				expect(html).to.include('<link rel="canonical" href="/login">');
+			});
+
+			it('A 302 status generates a "temporary redirect" through a short delay', async () => {
+				// https://developers.google.com/search/docs/crawling-indexing/301-redirects#metarefresh
+				const html = await fixture.readFile('/secret/index.html');
+				expect(html).to.include('content="2;url=/login"');
+			});
+
 			it('Includes the meta refresh tag in `redirect` config pages', async () => {
 				let html = await fixture.readFile('/one/index.html');
 				expect(html).to.include('http-equiv="refresh');
-				expect(html).to.include('url=/');
+				expect(html).to.include('url=/test');
 
 				html = await fixture.readFile('/two/index.html');
 				expect(html).to.include('http-equiv="refresh');
-				expect(html).to.include('url=/');
+				expect(html).to.include('url=/test');
 
 				html = await fixture.readFile('/three/index.html');
 				expect(html).to.include('http-equiv="refresh');
-				expect(html).to.include('url=/');
+				expect(html).to.include('url=/test');
+
+				html = await fixture.readFile('/index.html');
+				expect(html).to.include('http-equiv="refresh');
+				expect(html).to.include('url=/test');
 			});
 
 			it('Generates page for dynamic routes', async () => {
@@ -120,7 +147,7 @@ describe('Astro.redirect', () => {
 			it('Generates redirect pages for redirects created by middleware', async () => {
 				let html = await fixture.readFile('/middleware-redirect/index.html');
 				expect(html).to.include('http-equiv="refresh');
-				expect(html).to.include('url=/');
+				expect(html).to.include('url=/test');
 			});
 		});
 

diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js
@@ -4,6 +4,7 @@ import { createFs } from '../test-utils.js';
 import { createRouteManifest } from '../../../dist/core/routing/manifest/create.js';
 import { createDefaultDevSettings } from '../../../dist/core/config/index.js';
 import { fileURLToPath } from 'url';
+import { defaultLogging } from '../test-utils.js';
 
 const root = new URL('../../fixtures/alias/', import.meta.url);
 
@@ -59,6 +60,31 @@ describe('routing - createRouteManifest', () => {
 
 		expect(manifest.routes[1].route).to.equal('/blog/contributing');
 		expect(manifest.routes[1].type).to.equal('page');
-		expect(manifest.routes[2].route).to.equal('/blog/[...slug]');
+		expect(manifest.routes[3].route).to.equal('/blog/[...slug]');
+	});
+
+	it('static redirect route is prioritized over dynamic file route', async () => {
+		const fs = createFs(
+			{
+				'/src/pages/[...slug].astro': `<h1>test</h1>`
+			},
+			root
+		);
+		const settings = await createDefaultDevSettings(
+			{
+				trailingSlash: 'never',
+				redirects: {
+					'/foo': '/bar'
+				}
+			},
+			root
+		);
+		const manifest = createRouteManifest({
+			cwd: fileURLToPath(root),
+			settings,
+			fsMod: fs,
+		}, defaultLogging);
+
+		expect(manifest.routes[0].route).to.equal('/foo');
 	});
 });