fix: correctly serve ISR pages with trailing slash set to 'always'

.changeset/isr-trailing-slash.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/adapter-vercel': patch
+---
+
+fix: pass the requested pathname, including any trailing slash, to ISR functions

.github/workflows/platform-tests-vercel.yml

@@ -40,6 +40,15 @@ jobs:
           vercel-token: ${{ secrets.VERCEL_TOKEN }}
           vercel-org-id: ${{ secrets.VERCEL_ORG_ID }}
 
+      # avoid stale cache results when testing ISR routes
+      - name: Purge Vercel CDN Cache
+        shell: bash
+        env:
+          VERCEL_TOKEN: ${{ secrets.VERCEL_TOKEN }}
+          VERCEL_ORG_ID: ${{ secrets.VERCEL_ORG_ID }}
+          VERCEL_PROJECT_ID: ${{ secrets.VERCEL_PROJECT_ID_BASIC }}
+        run: vercel cache purge --type cdn --token="${{ secrets.VERCEL_TOKEN }}" --yes
+
       - uses: ./.github/actions/platform-test
         with:
           test-app-dir: packages/adapter-vercel/test/apps/basic

packages/adapter-vercel/files/serverless.js

@@ -10,8 +10,6 @@ await server.init({
 	read: createReadableStream
 });
 
-const DATA_SUFFIX = '/__data.json';
-
 export default {
 	/**
 	 * @param {Request} request
@@ -21,13 +19,10 @@ export default {
 		// If this is an ISR request, the requested pathname is encoded
 		// as a search parameter, so we need to extract it
 		const url = new URL(request.url);
-		let pathname = url.searchParams.get('__pathname');
+		const pathname = url.searchParams.get('__pathname');
 
 		if (pathname) {
-			// Optional routes' pathname replacements look like `/foo/$1/bar` which means we could end up with an url like /foo//bar
-			pathname = pathname.replace(/\/+/g, '/');
-
-			url.pathname = pathname + (url.pathname.endsWith(DATA_SUFFIX) ? DATA_SUFFIX : '');
+			url.pathname = pathname;
 			url.searchParams.delete('__pathname');
 
 			request = new Request(url, request);

packages/adapter-vercel/index.js

@@ -4,7 +4,7 @@ import process from 'node:process';
 import { fileURLToPath } from 'node:url';
 import { VERSION } from '@sveltejs/kit';
 import { nodeFileTrace } from '@vercel/nft';
-import { get_pathname, parse_isr_expiration, pattern_to_src, resolve_runtime } from './utils.js';
+import { parse_isr_expiration, pattern_to_src, resolve_runtime } from './utils.js';
 
 const INTERNAL = '![-]'; // this name is guaranteed not to conflict with user routes
 
@@ -230,6 +230,12 @@ const plugin = function (defaults = {}) {
 				});
 			}
 
+			// Vercel's filesystem phase serves a function at its own path, with or without a
+			// trailing slash, before the routes below are consulted. Static ISR routes live at
+			// their own path, so they must be routed before it to arrive with `__pathname`
+			/** @type {any[]} */
+			const static_isr_routes = [];
+
 			for (const route of builder.routes) {
 				if (is_prerendered(route)) continue;
 
@@ -254,7 +260,6 @@ const plugin = function (defaults = {}) {
 						fs.symlinkSync(`../${relative}`, `${base}/__data.json.func`);
 					}
 
-					const pathname = get_pathname(route);
 					const json = JSON.stringify(
 						{ ...isr, expiration: parse_isr_expiration(isr.expiration, route.id) },
 						null,
@@ -266,17 +271,23 @@ const plugin = function (defaults = {}) {
 						write(`${base}/__data.json.prerender-config.json`, json);
 					}
 
-					const q = `?__pathname=/${pathname}`;
+					const routes = route.segments.some((segment) => segment.dynamic)
+						? static_config.routes
+						: static_isr_routes;
 
-					static_config.routes.push({
-						src: src + '$',
-						dest: `/${isr_name}${q}`
+					// capture the requested pathname (minus the `^` anchor) as `__pathname`,
+					// since the function otherwise only sees its own path
+					const pathname = src.slice(1);
+
+					routes.push({
+						src: `^(${pathname})$`,
+						dest: `/${isr_name}?__pathname=$1`
 					});
 
 					if (has_page) {
-						static_config.routes.push({
-							src: src + '/__data.json$',
-							dest: `/${isr_name}/__data.json${q}`
+						routes.push({
+							src: `^(${pathname}/__data.json)$`,
+							dest: `/${isr_name}/__data.json?__pathname=$1`
 						});
 					}
 				} else {
@@ -318,6 +329,9 @@ const plugin = function (defaults = {}) {
 				}
 			}
 
+			const filesystem = static_config.routes.findIndex((route) => route.handle === 'filesystem');
+			static_config.routes.splice(filesystem, 0, ...static_isr_routes);
+
 			if (builder.config.router.resolution === 'server') {
 				// Create a separate serverless function just for server-side route resolution.
 				// By omitting all routes we're ensuring it's small (the routes will still be available

packages/adapter-vercel/test/apps/basic/build.test.js

@@ -21,7 +21,7 @@ test('__data.json prerender config is not generated for server-only route', () =
 	assert.ok(!fs.existsSync(`${functions}/__data.json.prerender-config.json`));
 });
 
-/** @type {{ routes: Array<{ src?: string }> }} */
+/** @type {{ routes: Array<{ src?: string, dest?: string, handle?: string }> }} */
 const config = JSON.parse(fs.readFileSync(`${output}/config.json`, 'utf8'));
 const route_sources = config.routes.flatMap((route) =>
 	typeof route.src === 'string' ? [route.src] : []
@@ -44,3 +44,20 @@ test('__data.json function exists for ISR page route', () => {
 test('__data.json function exists in Vercel routing configuration', () => {
 	assert.ok(isr_page_route_sources.some((src) => src.includes('__data.json')));
 });
+
+const filesystem = config.routes.findIndex((route) => route.handle === 'filesystem');
+
+test('ISR routes capture the requested pathname', () => {
+	const route = config.routes.find((route) => route.src === '^(/isr-trailing-slash/?)$');
+	assert.equal(route?.dest, '/isr-trailing-slash?__pathname=$1');
+});
+
+test('static ISR routes are matched before the filesystem', () => {
+	const index = config.routes.findIndex((route) => route.src === '^(/isr-trailing-slash/?)$');
+	assert.ok(index < filesystem);
+});
+
+test('dynamic ISR routes are matched after the filesystem', () => {
+	const index = config.routes.findIndex((route) => route.src === '^(/isr/([^/]+?)/?)$');
+	assert.ok(index > filesystem);
+});

packages/adapter-vercel/test/apps/basic/src/routes/isr-trailing-slash/+page.server.ts

@@ -0,0 +1,13 @@
+export const trailingSlash = 'always';
+
+export const config = {
+	isr: {
+		expiration: 60
+	}
+};
+
+export function load() {
+	return {
+		rendered_at: Date.now()
+	};
+}

packages/adapter-vercel/test/apps/basic/src/routes/isr-trailing-slash/+page.svelte

@@ -0,0 +1,6 @@
+<script lang="ts">
+	let { data } = $props();
+</script>
+
+<h1>ISR Trailing Slash Page</h1>
+<p id="rendered-at">{data.rendered_at}</p>

packages/adapter-vercel/test/apps/basic/test/test.ts

@@ -46,6 +46,20 @@ test('ISR route serves cached response', async ({ request }) => {
 	expect(first_rendered_at).toBe(second_rendered_at);
 });
 
+test('ISR page with trailingSlash always loads without errors', async ({ page, request }) => {
+	await page.goto('/isr-trailing-slash/');
+
+	expect(new URL(page.url()).pathname).toBe('/isr-trailing-slash/');
+	await expect(page.locator('h1')).toContainText('ISR Trailing Slash Page');
+
+	const rendered_at = await page.locator('#rendered-at').textContent();
+	await page.reload();
+	await expect(page.locator('#rendered-at')).toHaveText(String(rendered_at));
+
+	const response = await request.get('/isr-trailing-slash', { maxRedirects: 0 });
+	expect(response.status()).toBe(308);
+});
+
 test('ISR dynamic route serves cached response per slug', async ({ request }) => {
 	// warm the cache for /isr/alpha
 	const first = await request.get('/isr/alpha');
@@ -67,6 +81,10 @@ test('ISR dynamic route serves cached response per slug', async ({ request }) =>
 	expect(beta.ok()).toBe(true);
 	const beta_html = await beta.text();
 	expect(beta_html).toContain('ISR: beta');
+
+	// trailing slash is normalized rather than silently served
+	const slashed = await request.get('/isr/alpha/', { maxRedirects: 0 });
+	expect(slashed.status()).toBe(308);
 });
 
 test('prerendered page works', async ({ page }) => {

packages/adapter-vercel/utils.js

@@ -1,46 +1,5 @@
 import process from 'node:process';
 
-/** @param {import("@sveltejs/kit").RouteDefinition<any>} route */
-export function get_pathname(route) {
-	let i = 1;
-
-	const pathname = route.segments
-		.map((segment) => {
-			if (!segment.dynamic) {
-				return '/' + segment.content;
-			}
-
-			const parts = segment.content.split(/\[(.+?)\](?!\])/);
-
-			if (
-				parts.length === 3 &&
-				!parts[0] &&
-				!parts[2] &&
-				(parts[1].startsWith('...') || parts[1][0] === '[')
-			) {
-				// Special case: segment is a single optional or rest parameter.
-				// In that case we don't prepend a slash (also see comment in pattern_to_src).
-				return `$${i++}`;
-			} else {
-				return (
-					'/' +
-					parts
-						.map((content, j) => {
-							if (j % 2) {
-								return `$${i++}`;
-							} else {
-								return content;
-							}
-						})
-						.join('')
-				);
-			}
-		})
-		.join('');
-
-	return pathname[0] === '/' ? pathname.slice(1) : pathname;
-}
-
 /**
  * Adjusts the stringified route regex for Vercel's routing system
  * @param {string} pattern stringified route regex
@@ -57,14 +16,6 @@ export function pattern_to_src(pattern) {
 		src = '^/?';
 	}
 
-	// Move non-capturing groups that swallow slashes into their following capturing groups.
-	// This is necessary because during ISR we're using the regex to construct the __pathname
-	// query parameter: In case of a route like [required]/[...rest] we need to turn them
-	// into $1$2 and not $1/$2, because if [...rest] is empty, we don't want to have a trailing
-	// slash in the __pathname query parameter which wasn't there in the original URL, as that
-	// could result in a false trailing slash redirect in the SvelteKit runtime, leading to infinite redirects.
-	src = src.replace(/\(\?:\/\((.+?)\)\)/g, '(/$1)');
-
 	return src;
 }
 

packages/adapter-vercel/utils.spec.js

@@ -1,105 +1,11 @@
 import { assert, test, describe } from 'vitest';
-import { get_pathname, parse_isr_expiration, pattern_to_src, resolve_runtime } from './utils.js';
+import { parse_isr_expiration, pattern_to_src, resolve_runtime } from './utils.js';
 
 // workaround so that TypeScript doesn't follow that import which makes it pick up that file and then error on missing import aliases
 const { parse_route_id } = await import(
 	new URL('../kit/src/' + 'utils/routing.js', import.meta.url).href
 );
 
-/**
- * @param {import('@sveltejs/kit').RouteDefinition<any>['segments']} segments
- * @param {string} expected
- */
-function run_get_pathname_test(segments, expected) {
-	const route = /** @type {import('@sveltejs/kit').RouteDefinition<any>} */ ({ segments });
-	assert.equal(get_pathname(route), expected);
-}
-
-test('get_pathname for simple route', () => {
-	run_get_pathname_test([{ content: 'foo', dynamic: false, rest: false }], 'foo');
-});
-
-test('get_pathname for simple route with multiple segments', () => {
-	run_get_pathname_test(
-		[
-			{ content: 'foo', dynamic: false, rest: false },
-			{ content: 'bar', dynamic: false, rest: false }
-		],
-		'foo/bar'
-	);
-});
-
-test('get_pathname for route with parameters', () => {
-	run_get_pathname_test(
-		[
-			{ content: 'foo', dynamic: false, rest: false },
-			{ content: '[bar]', dynamic: true, rest: false }
-		],
-		'foo/$1'
-	);
-});
-
-test('get_pathname for route with parameters within segment', () => {
-	run_get_pathname_test(
-		[
-			{ content: 'foo-[bar]', dynamic: true, rest: false },
-			{ content: '[baz]-buz', dynamic: true, rest: false }
-		],
-		'foo-$1/$2-buz'
-	);
-});
-
-test('get_pathname for route with optional parameters within segment', () => {
-	run_get_pathname_test(
-		[
-			{ content: 'foo-[[bar]]', dynamic: true, rest: false },
-			{ content: '[[baz]]-buz', dynamic: true, rest: false }
-		],
-		'foo-$1/$2-buz'
-	);
-});
-
-test('get_pathname for route with rest parameter', () => {
-	run_get_pathname_test(
-		[
-			{ content: 'foo', dynamic: false, rest: false },
-			{ content: '[[...rest]]', dynamic: true, rest: true }
-		],
-		'foo$1'
-	);
-});
-
-test('get_pathname for route with required and rest parameter', () => {
-	run_get_pathname_test(
-		[
-			{ content: '[foo]', dynamic: true, rest: false },
-			{ content: '[...rest]', dynamic: true, rest: true }
-		],
-		'$1$2'
-	);
-});
-
-test('get_pathname for route with required and optional parameter', () => {
-	run_get_pathname_test(
-		[
-			{ content: '[foo]', dynamic: true, rest: false },
-			{ content: '[[optional]]', dynamic: true, rest: true }
-		],
-		'$1$2'
-	);
-});
-
-test('get_pathname for route with required and optional parameter', () => {
-	run_get_pathname_test(
-		[
-			{ content: '[foo]', dynamic: true, rest: false },
-			{ content: '[[...rest]]', dynamic: true, rest: true },
-			{ content: 'bar', dynamic: false, rest: false }
-		],
-		'$1$2/bar'
-	);
-});
-
 /**
  * @param {string} route_id
  * @param {string} expected
@@ -118,19 +24,19 @@ test('pattern_to_src for route with parameters', () => {
 });
 
 test('pattern_to_src for route with optional parameters', () => {
-	run_pattern_to_src_test('/foo/[[bar]]', '^/foo(/[^/]+)?/?');
+	run_pattern_to_src_test('/foo/[[bar]]', '^/foo(?:/([^/]+))?/?');
 });
 
 test('pattern_to_src for route with optional parameter in the middle', () => {
-	run_pattern_to_src_test('/foo/[[bar]]/baz', '^/foo(/[^/]+)?/baz/?');
+	run_pattern_to_src_test('/foo/[[bar]]/baz', '^/foo(?:/([^/]+))?/baz/?');
 });
 
 test('pattern_to_src for route with rest parameter', () => {
-	run_pattern_to_src_test('/foo/[...bar]', '^/foo(/[^]*)?/?');
+	run_pattern_to_src_test('/foo/[...bar]', '^/foo(?:/([^]*))?/?');
 });
 
 test('pattern_to_src for route with rest parameter in the middle', () => {
-	run_pattern_to_src_test('/foo/[...bar]/baz', '^/foo(/[^]*)?/baz/?');
+	run_pattern_to_src_test('/foo/[...bar]/baz', '^/foo(?:/([^]*))?/baz/?');
 });
 
 describe('parse_isr_expiration', () => {