Skip to content

Commit

Permalink
Allow chained optional parameters (#7753)
Browse files Browse the repository at this point in the history 
* add failing test for #7548

* refactor

* fix

* always add trailing slash, that logic is way out of date

* fix tests

* DRY out

* track more param info

* check for missing matcher at manifest creation time

* working, i think

* shuffle around

* ok try this

* update tests

* fix tests

* lint

* fix

* changeset

* unit tests

* make the code a bit more comprehensible

* Update .changeset/forty-icons-switch.md

* fix another case
  • Loading branch information
@Rich-Harris
Rich-Harris committed Nov 22, 2022
1 parent 50de980 commit dfaf3da
Show file tree
Hide file tree
Showing 12 changed files with 160 additions and 122 deletions.
5 changes: 5 additions & 0 deletions .changeset/forty-icons-switch.md
View file
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

Roll over non-matching optional parameters instead of 404ing
8 changes: 3 additions & 5 deletions packages/kit/src/core/generate_manifest/index.js
View file
Expand Up @@ -99,18 +99,16 @@ export function generate_manifest({ build_data, relative_path, routes, format =
],
routes: [
${routes.map(route => {
route.types.forEach(type => {
if (type) matchers.add(type);
route.params.forEach(param => {
if (param.matcher) matchers.add(param.matcher);
});
if (!route.page && !route.endpoint) return;
return `{
id: ${s(route.id)},
pattern: ${route.pattern},
names: ${s(route.names)},
types: ${s(route.types)},
optional: ${s(route.optional)},
params: ${s(route.params)},
page: ${route.page ? `{ layouts: ${get_nodes(route.page.layouts)}, errors: ${get_nodes(route.page.errors)}, leaf: ${reindexed.get(route.page.leaf)} }` : 'null'},
endpoint: ${route.endpoint ? loader(join_relative(relative_path, resolve_symlinks(build_data.server.vite_manifest, route.endpoint.file).chunk.file)) : 'null'}
}`;
Expand Down
18 changes: 11 additions & 7 deletions packages/kit/src/core/sync/create_manifest_data/index.js
View file
Expand Up @@ -24,6 +24,14 @@ export default function create_manifest_data({
const matchers = create_matchers(config, cwd);
const { nodes, routes } = create_routes_and_nodes(cwd, config, fallback);

for (const route of routes) {
for (const param of route.params) {
if (param.matcher && !matchers[param.matcher]) {
throw new Error(`No matcher found for parameter '${param.matcher}' in route ${route.id}`);
}
}
}

return {
assets,
matchers,
Expand Down Expand Up @@ -153,7 +161,7 @@ function create_routes_and_nodes(cwd, config, fallback) {
);
}

const { pattern, names, types, optional } = parse_route_id(id);
const { pattern, params } = parse_route_id(id);

/** @type {import('types').RouteData} */
const route = {
Expand All @@ -162,9 +170,7 @@ function create_routes_and_nodes(cwd, config, fallback) {

segment,
pattern,
names,
types,
optional,
params,

layout: null,
error: null,
Expand Down Expand Up @@ -273,9 +279,7 @@ function create_routes_and_nodes(cwd, config, fallback) {
id: '/',
segment: '',
pattern: /^$/,
names: [],
types: [],
optional: [],
params: [],
parent: null,
layout: null,
error: null,
Expand Down
12 changes: 6 additions & 6 deletions packages/kit/src/core/sync/create_manifest_data/index.spec.js
View file
Expand Up @@ -87,7 +87,7 @@ test('creates routes', () => {
},
{
id: '/blog.json',
pattern: '/^/blog.json$/',
pattern: '/^/blog.json/?$/',
endpoint: { file: 'samples/basic/blog.json/+server.js' }
},
{
Expand All @@ -97,7 +97,7 @@ test('creates routes', () => {
},
{
id: '/blog/[slug].json',
pattern: '/^/blog/([^/]+?).json$/',
pattern: '/^/blog/([^/]+?).json/?$/',
endpoint: {
file: 'samples/basic/blog/[slug].json/+server.ts'
}
Expand Down Expand Up @@ -308,7 +308,7 @@ test('allows rest parameters inside segments', () => {
},
{
id: '/[...rest].json',
pattern: '/^/(.*?).json$/',
pattern: '/^/(.*?).json/?$/',
endpoint: {
file: 'samples/rest-prefix-suffix/[...rest].json/+server.js'
}
Expand Down Expand Up @@ -408,7 +408,7 @@ test('allows multiple slugs', () => {
assert.equal(routes.filter((route) => route.endpoint).map(simplify_route), [
{
id: '/[file].[ext]',
pattern: '/^/([^/]+?).([^/]+?)$/',
pattern: '/^/([^/]+?).([^/]+?)/?$/',
endpoint: {
file: 'samples/multiple-slugs/[file].[ext]/+server.js'
}
Expand Down Expand Up @@ -467,7 +467,7 @@ test('works with custom extensions', () => {
},
{
id: '/blog.json',
pattern: '/^/blog.json$/',
pattern: '/^/blog.json/?$/',
endpoint: {
file: 'samples/custom-extension/blog.json/+server.js'
}
Expand All @@ -479,7 +479,7 @@ test('works with custom extensions', () => {
},
{
id: '/blog/[slug].json',
pattern: '/^/blog/([^/]+?).json$/',
pattern: '/^/blog/([^/]+?).json/?$/',
endpoint: {
file: 'samples/custom-extension/blog/[slug].json/+server.js'
}
Expand Down
8 changes: 4 additions & 4 deletions packages/kit/src/core/sync/write_types/index.js
View file
Expand Up @@ -195,8 +195,8 @@ function update_types(config, routes, route, to_delete = new Set()) {
// Makes sure a type is "repackaged" and therefore more readable
declarations.push('type Expand<T> = T extends infer O ? { [K in keyof O]: O[K] } : never;');
declarations.push(
`type RouteParams = { ${route.names
.map((param, idx) => `${param}${route.optional[idx] ? '?' : ''}: string`)
`type RouteParams = { ${route.params
.map((param) => `${param.name}${param.optional ? '?' : ''}: string`)
.join('; ')} }`
);

Expand Down Expand Up @@ -270,8 +270,8 @@ function update_types(config, routes, route, to_delete = new Set()) {
if (leaf) {
if (leaf.route.page) ids.push(`"${leaf.route.id}"`);

for (const name of leaf.route.names) {
layout_params.add(name);
for (const param of leaf.route.params) {
layout_params.add(param.name);
}

ensureProxies(page, leaf.proxies);
Expand Down
4 changes: 1 addition & 3 deletions packages/kit/src/exports/vite/dev/index.js
View file
Expand Up @@ -156,9 +156,7 @@ export async function dev(vite, vite_config, svelte_config) {
return {
id: route.id,
pattern: route.pattern,
names: route.names,
types: route.types,
optional: route.optional,
params: route.params,
page: route.page,
endpoint: endpoint
? async () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/kit/src/runtime/client/parse.js
View file
Expand Up @@ -11,14 +11,14 @@ export function parse(nodes, server_loads, dictionary, matchers) {
const layouts_with_server_load = new Set(server_loads);

return Object.entries(dictionary).map(([id, [leaf, layouts, errors]]) => {
const { pattern, names, types, optional } = parse_route_id(id);
const { pattern, params } = parse_route_id(id);

const route = {
id,
/** @param {string} path */
exec: (path) => {
const match = pattern.exec(path);
if (match) return exec(match, { names, types, optional }, matchers);
if (match) return exec(match, params, matchers);
},
errors: [1, ...(errors || [])].map((n) => nodes[n]),
layouts: [0, ...(layouts || [])].map(create_layout_loader),
Expand Down
2 changes: 1 addition & 1 deletion packages/kit/src/runtime/server/index.js
View file
Expand Up @@ -77,7 +77,7 @@ export async function respond(request, options, state) {
const match = candidate.pattern.exec(decoded);
if (!match) continue;

const matched = exec(match, candidate, matchers);
const matched = exec(match, candidate.params, matchers);
if (matched) {
route = candidate;
params = decode_params(matched);
Expand Down
127 changes: 80 additions & 47 deletions packages/kit/src/utils/routing.js
View file
Expand Up @@ -5,44 +5,40 @@ const param_pattern = /^(\[)?(\.\.\.)?(\w+)(?:=(\w+))?(\])?$/;
* @param {string} id
*/
export function parse_route_id(id) {
/** @type {string[]} */
const names = [];

/** @type {string[]} */
const types = [];

/** @type {boolean[]} */
const optional = [];

// `/foo` should get an optional trailing slash, `/foo.json` should not
// const add_trailing_slash = !/\.[a-z]+$/.test(key);
let add_trailing_slash = true;
/** @type {import('types').RouteParam[]} */
const params = [];

const pattern =
id === '/'
? /^\/$/
: new RegExp(
`^${get_route_segments(id)
.map((segment, i, segments) => {
.map((segment) => {
// special case — /[...rest]/ could contain zero segments
const rest_match = /^\[\.\.\.(\w+)(?:=(\w+))?\]$/.exec(segment);
if (rest_match) {
names.push(rest_match[1]);
types.push(rest_match[2]);
optional.push(false);
params.push({
name: rest_match[1],
matcher: rest_match[2],
optional: false,
rest: true,
chained: true
});
return '(?:/(.*))?';
}
// special case — /[[optional]]/ could contain zero segments
const optional_match = /^\[\[(\w+)(?:=(\w+))?\]\]$/.exec(segment);
if (optional_match) {
names.push(optional_match[1]);
types.push(optional_match[2]);
optional.push(true);
params.push({
name: optional_match[1],
matcher: optional_match[2],
optional: true,
rest: false,
chained: true
});
return '(?:/([^/]+))?';
}
const is_last = i === segments.length - 1;
if (!segment) {
return;
}
Expand Down Expand Up @@ -73,29 +69,31 @@ export function parse_route_id(id) {
);
}
const [, is_optional, is_rest, name, type] = match;
const [, is_optional, is_rest, name, matcher] = match;
// It's assumed that the following invalid route id cases are already checked
// - unbalanced brackets
// - optional param following rest param
names.push(name);
types.push(type);
optional.push(!!is_optional);
params.push({
name,
matcher,
optional: !!is_optional,
rest: !!is_rest,
chained: is_rest ? i === 1 && parts[0] === '' : false
});
return is_rest ? '(.*?)' : is_optional ? '([^/]*)?' : '([^/]+?)';
}
if (is_last && content.includes('.')) add_trailing_slash = false;
return escape(content);
})
.join('');
return '/' + result;
})
.join('')}${add_trailing_slash ? '/?' : ''}$`
.join('')}/?$`
);

return { pattern, names, types, optional };
return { pattern, params };
}

/**
Expand All @@ -119,35 +117,70 @@ export function get_route_segments(route) {

/**
* @param {RegExpMatchArray} match
* @param {{
* names: string[];
* types: string[];
* optional: boolean[];
* }} candidate
* @param {import('types').RouteParam[]} params
* @param {Record<string, import('types').ParamMatcher>} matchers
*/
export function exec(match, { names, types, optional }, matchers) {
export function exec(match, params, matchers) {
/** @type {Record<string, string>} */
const params = {};
const result = {};

const values = match.slice(1);

for (let i = 0; i < names.length; i += 1) {
const name = names[i];
const type = types[i];
let value = match[i + 1];
let buffered = '';

if (value || !optional[i]) {
if (type) {
const matcher = matchers[type];
if (!matcher) throw new Error(`Missing "${type}" param matcher`); // TODO do this ahead of time?
for (let i = 0; i < params.length; i += 1) {
const param = params[i];
let value = values[i];

if (param.chained && param.rest && buffered) {
// in the `[[lang=lang]]/[...rest]` case, if `lang` didn't
// match, we roll it over into the rest value
value = value ? buffered + '/' + value : buffered;
}

if (!matcher(value)) return;
buffered = '';

if (value === undefined) {
// if `value` is undefined, it means this is
// an optional or rest parameter
if (param.rest) result[param.name] = '';
} else {
if (param.matcher && !matchers[param.matcher](value)) {
// in the `/[[a=b]]/[[c=d]]` case, if the value didn't satisfy the `b` matcher,
// try again with the next segment by shifting values rightwards
if (param.optional && param.chained) {
// @ts-expect-error TypeScript is... wrong
let j = values.indexOf(undefined, i);

if (j === -1) {
// we can't shift values any further, so hang on to this value
// so it can be rolled into a subsequent `[...rest]` param
const next = params[i + 1];
if (next?.rest && next.chained) {
buffered = value;
} else {
return;
}
}

while (j >= i) {
values[j] = values[j - 1];
j -= 1;
}

continue;
}

// otherwise, if the matcher returns `false`, the route did not match
return;
}

params[name] = value ?? '';
result[param.name] = value;
}
}

return params;
if (buffered) return;
return result;
}

/** @param {string} str */
Expand Down

0 comments on commit dfaf3da

Please sign in to comment.