diff --git a/.changeset/isr-trailing-slash.md b/.changeset/isr-trailing-slash.md new file mode 100644 index 000000000000..bc116043a396 --- /dev/null +++ b/.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 diff --git a/.github/workflows/platform-tests-vercel.yml b/.github/workflows/platform-tests-vercel.yml index 13411077f5d0..b789b2bf01cd 100644 --- a/.github/workflows/platform-tests-vercel.yml +++ b/.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 diff --git a/packages/adapter-vercel/files/serverless.js b/packages/adapter-vercel/files/serverless.js index d6aa640edf16..f4c527bd5210 100644 --- a/packages/adapter-vercel/files/serverless.js +++ b/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); diff --git a/packages/adapter-vercel/index.js b/packages/adapter-vercel/index.js index 99a7b67c6ec5..200ce5c06b1b 100644 --- a/packages/adapter-vercel/index.js +++ b/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 diff --git a/packages/adapter-vercel/test/apps/basic/build.test.js b/packages/adapter-vercel/test/apps/basic/build.test.js index 06b6de2661b8..c9fd8cdc1f94 100644 --- a/packages/adapter-vercel/test/apps/basic/build.test.js +++ b/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); +}); diff --git a/packages/adapter-vercel/test/apps/basic/src/routes/isr-trailing-slash/+page.server.ts b/packages/adapter-vercel/test/apps/basic/src/routes/isr-trailing-slash/+page.server.ts new file mode 100644 index 000000000000..955c0d986b74 --- /dev/null +++ b/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() + }; +} diff --git a/packages/adapter-vercel/test/apps/basic/src/routes/isr-trailing-slash/+page.svelte b/packages/adapter-vercel/test/apps/basic/src/routes/isr-trailing-slash/+page.svelte new file mode 100644 index 000000000000..01714bc82a70 --- /dev/null +++ b/packages/adapter-vercel/test/apps/basic/src/routes/isr-trailing-slash/+page.svelte @@ -0,0 +1,6 @@ + + +

ISR Trailing Slash Page

+

{data.rendered_at}

diff --git a/packages/adapter-vercel/test/apps/basic/test/test.ts b/packages/adapter-vercel/test/apps/basic/test/test.ts index 1f54da741c61..a4b284ea699d 100644 --- a/packages/adapter-vercel/test/apps/basic/test/test.ts +++ b/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 }) => { diff --git a/packages/adapter-vercel/utils.js b/packages/adapter-vercel/utils.js index e6570fd0e0fd..ed9253c8474a 100644 --- a/packages/adapter-vercel/utils.js +++ b/packages/adapter-vercel/utils.js @@ -1,46 +1,5 @@ import process from 'node:process'; -/** @param {import("@sveltejs/kit").RouteDefinition} 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; } diff --git a/packages/adapter-vercel/utils.spec.js b/packages/adapter-vercel/utils.spec.js index 72c9b7fa2ff4..d382bd5f6d00 100644 --- a/packages/adapter-vercel/utils.spec.js +++ b/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['segments']} segments - * @param {string} expected - */ -function run_get_pathname_test(segments, expected) { - const route = /** @type {import('@sveltejs/kit').RouteDefinition} */ ({ 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', () => {