Skip to content

fix: support optional params in Netlify split mode #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
anurag6569201 wants to merge 1 commit into
base: qa/agent-sveltejs-kit/pr-07-17020/base
Choose a base branch
from
Open

fix: support optional params in Netlify split mode #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fresh-pandas-smile.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/adapter-netlify': patch
---

fix: route requests with omitted optional parameters to split serverless functions
11 changes: 7 additions & 4 deletions packages/adapter-netlify/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,13 @@ function generate_serverless_functions({ builder, publish, split }) {

// The parts should conform to URLPattern syntax
// https://docs.netlify.com/build/functions/get-started/?fn-language=ts&data-tab=TypeScript#route-requests
for (const segment of route.segments) {
for (const [i, segment] of route.segments.entries()) {
if (segment.rest) {
parts.push('*');
} else if (segment.dynamic) {
// URLPattern requires params to start with letters
parts.push(`:param${parts.length}`);
const optional = /^\[\[.+\]\]$/.test(segment.content) ? '?' : '';
parts.push(`:param${i}${optional}`);
} else {
parts.push(segment.content);
}
Expand All @@ -159,13 +160,15 @@ function generate_serverless_functions({ builder, publish, split }) {
// Netlify handles trailing slashes for us, so we don't need to include them in the pattern
const pattern = `/${parts.join('/')}`;
const name =
FUNCTION_PREFIX + (parts.join('-').replace(/[:.]/g, '_').replace('*', '__rest') || 'index');
FUNCTION_PREFIX +

@shipwright-agent shipwright-agent Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · CRITICAL

The function name generation uses 'parts.join('-')' where 'parts' now contains ':param1?'.

Impact: The function name generation uses 'parts.join('-')' where 'parts' now contains ':param1?'. The '.replace(/[:.]/g, '_')' runs before '.replace(/?/g, '')', so the '?' is removed, but the test expects the file 'sveltekit-collection-_param1-article.mjs'. However, for a route with multiple optional params or a rest segment combined with optional params, the '?' removal can create name collisions. More critically, the 's…

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

(parts.join('-').replace(/[:.]/g, '_').replace(/\?/g, '').replace(/\*/g, '__rest') ||
'index');

// skip routes with identical patterns, they were already folded into another function
if (seen.has(pattern)) continue;

const patterns = [pattern, `${pattern === '/' ? '' : pattern}/__data.json`];
patterns.forEach((p) => seen.add(p));
patterns.forEach((pattern) => seen.add(pattern));

@shipwright-agent shipwright-agent Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The variable shadowing in 'patterns.forEach((pattern) => seen.add(pattern))' reuses the name 'pattern' for the loop parameter, shadowing the outer 'const pattern'.

Impact: The variable shadowing in 'patterns.forEach((pattern) => seen.add(pattern))' reuses the name 'pattern' for the loop parameter, shadowing the outer 'const pattern'. This is confusing and error-prone; a future maintainer modifying the loop body may inadvertently reference the wrong 'pattern'. The original code used 'p' to avoid this. This change reduces clarity without functional benefit.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.


// figure out which lower priority routes should be considered fallbacks
for (let j = i + 1; j < builder.routes.length; j += 1) {
Expand Down
4 changes: 4 additions & 0 deletions ...dapter-netlify/test/apps/split/src/routes/collection/[[optional]]/article/+page.server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/** @type {import('./$types').PageServerLoad} */
export function load({ params }) {
return { optional: params.optional ?? null };
}
6 changes: 6 additions & 0 deletions ...s/adapter-netlify/test/apps/split/src/routes/collection/[[optional]]/article/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<script>
let { data } = $props();
</script>

<!-- Netlify uses URLPattern for routing; verify direct SSR works whether the optional param is present or omitted -->
<p>optional: {data.optional ?? 'none'}</p>
18 changes: 17 additions & 1 deletion packages/adapter-netlify/test/apps/split/test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,19 @@ import fs from 'node:fs';
import path from 'node:path';
import { expect, test } from '@playwright/test';

test('routes to routes with dynamic params', async ({ page }) => {
test('routes with dynamic params', async ({ page }) => {
await page.goto('/dynamic/123');
await expect(page.locator('p')).toHaveText('id: 123');
});

test('routes with optional params', async ({ page }) => {
await page.goto('/collection/article');
await expect(page.locator('p')).toHaveText('optional: none');

await page.goto('/collection/value/article');
await expect(page.locator('p')).toHaveText('optional: value');
});

test('client-side navigation fetches server load function data', async ({ page }) => {
await page.goto('/dynamic');
await page.click('a');
Expand All @@ -27,6 +35,14 @@ test('split generates multiple function files', () => {
const functions_dir = path.resolve(import.meta.dirname, '../.netlify/v1/functions');
const files = fs.readdirSync(functions_dir).filter((f) => f.startsWith('sveltekit-'));
expect(files.length).toBeGreaterThan(1);

const optional_route = fs.readFileSync(
path.join(functions_dir, 'sveltekit-collection-_param1-article.mjs'),
'utf-8'
);
expect(optional_route).toContain(
'path: ["/collection/:param1?/article", "/collection/:param1?/article/__data.json"]'
);
});

test('_redirects are copied to publish directory', () => {
Expand Down