Properly support trailingSlash: never with a base (#5358)

* Properly support trailingSlash: never with a base * adding a changeset * Pass through the base * only mess with pathname when trailingSlash === 'never' * maybe fixes stuff * Update based on review notes
withastro · Nov 11, 2022 · 9eee0f0 · 9eee0f0
1 parent 4d425b4
commit 9eee0f0
Show file tree

Hide file tree

Showing 10 changed files with 179 additions and 16 deletions.
diff --git a/.changeset/kind-icons-destroy.md b/.changeset/kind-icons-destroy.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Properly support trailingSlash: never with a base
diff --git a/packages/astro/src/core/config/schema.ts b/packages/astro/src/core/config/schema.ts
@@ -77,8 +77,7 @@ export const AstroConfigSchema = z.object({
 	base: z
 		.string()
 		.optional()
-		.default(ASTRO_CONFIG_DEFAULTS.base)
-		.transform((val) => prependForwardSlash(appendForwardSlash(trimSlashes(val)))),
+		.default(ASTRO_CONFIG_DEFAULTS.base),
 	trailingSlash: z
 		.union([z.literal('always'), z.literal('never'), z.literal('ignore')])
 		.optional()
@@ -326,6 +325,12 @@ export function createRelativeSchema(cmd: string, fileProtocolRoot: URL) {
 		) {
 			config.build.client = new URL('./dist/client/', config.outDir);
 		}
+		const trimmedBase = trimSlashes(config.base);
+		if(trimmedBase.length && config.trailingSlash === 'never') {
+      config.base = prependForwardSlash(trimmedBase);
+    } else {
+      config.base = prependForwardSlash(appendForwardSlash(trimmedBase));
+    }
 		return config;
 	});
 

diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts
@@ -61,7 +61,7 @@ function getParts(part: string, file: string) {
 	return result;
 }
 
-function getPattern(segments: RoutePart[][], addTrailingSlash: AstroConfig['trailingSlash']) {
+function getPattern(segments: RoutePart[][], base: string, addTrailingSlash: AstroConfig['trailingSlash']) {
 	const pathname = segments
 		.map((segment) => {
 			if (segment.length === 1 && segment[0].spread) {
@@ -93,7 +93,11 @@ function getPattern(segments: RoutePart[][], addTrailingSlash: AstroConfig['trai
 
 	const trailing =
 		addTrailingSlash && segments.length ? getTrailingSlashPattern(addTrailingSlash) : '$';
-	return new RegExp(`^${pathname || '\\/'}${trailing}`);
+	let initial = '\\/';
+	if(addTrailingSlash === 'never' && base !== '/') {
+		initial = '';
+	}
+	return new RegExp(`^${pathname || initial}${trailing}`);
 }
 
 function getTrailingSlashPattern(addTrailingSlash: AstroConfig['trailingSlash']): string {
@@ -306,7 +310,7 @@ export function createRouteManifest(
 				components.push(item.file);
 				const component = item.file;
 				const trailingSlash = item.isPage ? settings.config.trailingSlash : 'never';
-				const pattern = getPattern(segments, trailingSlash);
+				const pattern = getPattern(segments, settings.config.base, trailingSlash);
 				const generate = getRouteGenerator(segments, trailingSlash);
 				const pathname = segments.every((segment) => segment.length === 1 && !segment[0].dynamic)
 					? `/${segments.map((segment) => segment[0].content).join('/')}`
@@ -367,7 +371,7 @@ export function createRouteManifest(
 			const isPage = type === 'page';
 			const trailingSlash = isPage ? config.trailingSlash : 'never';
 
-			const pattern = getPattern(segments, trailingSlash);
+			const pattern = getPattern(segments, settings.config.base, trailingSlash);
 			const generate = getRouteGenerator(segments, trailingSlash);
 			const pathname = segments.every((segment) => segment.length === 1 && !segment[0].dynamic)
 				? `/${segments.map((segment) => segment[0].content).join('/')}`

diff --git a/packages/astro/src/vite-plugin-astro-server/base.ts b/packages/astro/src/vite-plugin-astro-server/base.ts
@@ -15,14 +15,15 @@ export function baseMiddleware(
 	const site = config.site ? new URL(config.base, config.site) : undefined;
 	const devRootURL = new URL(config.base, 'http://localhost');
 	const devRoot = site ? site.pathname : devRootURL.pathname;
+	const devRootReplacement = devRoot.endsWith('/') ? '/' : '';
 
 	return function devBaseMiddleware(req, res, next) {
 		const url = req.url!;
 
 		const pathname = decodeURI(new URL(url, 'http://localhost').pathname);
 
 		if (pathname.startsWith(devRoot)) {
-			req.url = url.replace(devRoot, '/');
+			req.url = url.replace(devRoot, devRootReplacement);
 			return next();
 		}
 

diff --git a/packages/astro/src/vite-plugin-astro-server/plugin.ts b/packages/astro/src/vite-plugin-astro-server/plugin.ts
@@ -50,8 +50,10 @@ export default function createVitePluginAstroServer({
 					});
 				}
 				viteServer.middlewares.use(async (req, res) => {
-					if (!req.url || !req.method) {
-						throw new Error('Incomplete request');
+					if (req.url === undefined || !req.method) {
+						res.writeHead(500, 'Incomplete request');
+						res.end();
+						return;
 					}
 					handleRequest(env, manifest, serverController, req, res);
 				});

diff --git a/packages/astro/src/vite-plugin-astro-server/request.ts b/packages/astro/src/vite-plugin-astro-server/request.ts
@@ -7,6 +7,7 @@ import { collectErrorMetadata } from '../core/errors/dev/index.js';
 import { createSafeError } from '../core/errors/index.js';
 import { error } from '../core/logger/core.js';
 import * as msg from '../core/messages.js';
+import { removeTrailingForwardSlash } from '../core/path.js';
 import { runWithErrorHandling } from './controller.js';
 import { handle500Response } from './response.js';
 import { handleRoute, matchRoute } from './route.js';
@@ -23,11 +24,17 @@ export async function handleRequest(
 	const { config } = settings;
 	const origin = `${moduleLoader.isHttps() ? 'https' : 'http'}://${req.headers.host}`;
 	const buildingToSSR = config.output === 'server';
+
 	const url = new URL(origin + req.url);
-	const pathname = decodeURI(url.pathname);
+	let pathname: string;
+	if(config.trailingSlash === 'never' && !req.url) {
+		pathname = '';
+	} else {
+		pathname = decodeURI(url.pathname);
+	}
 
 	// Add config.base back to url before passing it to SSR
-	url.pathname = config.base.substring(0, config.base.length - 1) + url.pathname;
+	url.pathname = removeTrailingForwardSlash(config.base) + url.pathname;
 
 	// HACK! @astrojs/image uses query params for the injected route in `dev`
 	if (!buildingToSSR && pathname !== '/_image') {

diff --git a/packages/astro/test/astro-global.test.js b/packages/astro/test/astro-global.test.js
@@ -25,7 +25,10 @@ describe('Astro Global', () => {
 		});
 
 		it('Astro.request.url', async () => {
-			const html = await fixture.fetch('/blog/?foo=42').then((res) => res.text());
+			const res = await await fixture.fetch('/blog/?foo=42');
+			expect(res.status).to.equal(200);
+
+			const html = await res.text();
 			const $ = cheerio.load(html);
 			expect($('#pathname').text()).to.equal('/blog/');
 			expect($('#searchparams').text()).to.equal('{}');

diff --git a/packages/astro/test/units/dev/base.test.js b/packages/astro/test/units/dev/base.test.js
@@ -0,0 +1,106 @@
+import { expect } from 'chai';
+
+import { runInContainer } from '../../../dist/core/dev/index.js';
+import { createFs, createRequestAndResponse } from '../test-utils.js';
+
+const root = new URL('../../fixtures/alias/', import.meta.url);
+
+describe('base configuration', () => {
+	describe('with trailingSlash: "never"', () => {
+		describe('index route', () => {
+			it('Requests that include a trailing slash 404', async () => {
+				const fs = createFs({
+					'/src/pages/index.astro': `<h1>testing</h1>`,
+				}, root);
+
+				await runInContainer({
+					fs,
+					root,
+					userConfig: {
+						base: '/docs',
+						trailingSlash: 'never',
+					},
+				}, async (container) => {
+					const { req, res, done } = createRequestAndResponse({
+						method: 'GET',
+						url: '/docs/',
+					});
+					container.handle(req, res);
+					await done;
+					expect(res.statusCode).to.equal(404);
+				});
+			});
+
+			it('Requests that exclude a trailing slash 200', async () => {
+				const fs = createFs({
+					'/src/pages/index.astro': `<h1>testing</h1>`,
+				}, root);
+
+				await runInContainer({
+					fs,
+					root,
+					userConfig: {
+						base: '/docs',
+						trailingSlash: 'never',
+					},
+				}, async (container) => {
+					const { req, res, done } = createRequestAndResponse({
+						method: 'GET',
+						url: '/docs',
+					});
+					container.handle(req, res);
+					await done;
+					expect(res.statusCode).to.equal(200);
+				});
+			});
+		});
+
+		describe('sub route', () => {
+			it('Requests that include a trailing slash 404', async () => {
+				const fs = createFs({
+					'/src/pages/sub/index.astro': `<h1>testing</h1>`,
+				}, root);
+
+				await runInContainer({
+					fs,
+					root,
+					userConfig: {
+						base: '/docs',
+						trailingSlash: 'never',
+					},
+				}, async (container) => {
+					const { req, res, done } = createRequestAndResponse({
+						method: 'GET',
+						url: '/docs/sub/',
+					});
+					container.handle(req, res);
+					await done;
+					expect(res.statusCode).to.equal(404);
+				});
+			});
+
+			it('Requests that exclude a trailing slash 200', async () => {
+				const fs = createFs({
+					'/src/pages/sub/index.astro': `<h1>testing</h1>`,
+				}, root);
+
+				await runInContainer({
+					fs,
+					root,
+					userConfig: {
+						base: '/docs',
+						trailingSlash: 'never',
+					},
+				}, async (container) => {
+					const { req, res, done } = createRequestAndResponse({
+						method: 'GET',
+						url: '/docs/sub',
+					});
+					container.handle(req, res);
+					await done;
+					expect(res.statusCode).to.equal(200);
+				});
+			});
+		});
+	});
+});
diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js
@@ -0,0 +1,28 @@
+import { expect } from 'chai';
+
+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';
+
+const root = new URL('../../fixtures/alias/', import.meta.url);
+
+describe('routing - createRouteManifest', () => {
+	it('using trailingSlash: "never" does not match the index route when it contains a trailing slash', async () => {
+		const fs = createFs({
+			'/src/pages/index.astro': `<h1>test</h1>`,
+		}, root);
+		const settings = await createDefaultDevSettings({
+			base: '/search',
+			trailingSlash: 'never'
+		}, root);
+		const manifest = createRouteManifest({
+			cwd: fileURLToPath(root),
+			settings,
+			fsMod: fs
+		});
+		const [{ pattern }] = manifest.routes;
+		expect(pattern.test('')).to.equal(true);
+		expect(pattern.test('/')).to.equal(false);
+	});
+});
diff --git a/packages/astro/test/units/vite-plugin-astro-server/request.test.js b/packages/astro/test/units/vite-plugin-astro-server/request.test.js
@@ -26,7 +26,7 @@ describe('vite-plugin-astro-server', () => {
 		it('renders a request', async () => {
 			const env = await createDevEnvironment({
 				loader: createLoader({
-					import(id) {
+					import() {
 						const Page = createComponent(() => {
 							return render`<div id="test">testing</div>`;
 						});
@@ -53,11 +53,13 @@ describe('vite-plugin-astro-server', () => {
 
 			try {
 				await handleRequest(env, manifest, controller, req, res);
-				const html = await text();
-				expect(html).to.include('<div id="test">');
 			} catch (err) {
-				expect(err).to.be.undefined();
+				expect(err.message).to.be.undefined();
 			}
+
+			const html = await text();
+			expect(res.statusCode).to.equal(200);
+			expect(html).to.include('<div id="test">');
 		});
 	});
 });