From a5ae72ac5140f0d8d0d43fb55f2d2ced6e072017 Mon Sep 17 00:00:00 2001 From: Eduardo San Martin Morote Date: Mon, 25 Aug 2025 16:17:46 +0200 Subject: [PATCH] refactor: simplify param parsers and allow trailingSlash in dynamic params --- .../matchers/matcher-pattern.spec.ts | 84 ++++++++++++------- .../matchers/matcher-pattern.test-d.ts | 10 +-- .../matchers/matcher-pattern.ts | 70 +++++++++++----- .../router/src/experimental/router.spec.ts | 10 +-- 4 files changed, 113 insertions(+), 61 deletions(-) diff --git a/packages/router/src/experimental/route-resolver/matchers/matcher-pattern.spec.ts b/packages/router/src/experimental/route-resolver/matchers/matcher-pattern.spec.ts index cf509784..d0bfbeba 100644 --- a/packages/router/src/experimental/route-resolver/matchers/matcher-pattern.spec.ts +++ b/packages/router/src/experimental/route-resolver/matchers/matcher-pattern.spec.ts @@ -132,7 +132,7 @@ describe('MatcherPatternPathDynamic', () => { /^\/teams\/([^/]+?)\/b$/i, { // all defaults - teamId: {}, + teamId: [{}], }, ['teams', 1, 'b'] ) @@ -153,7 +153,7 @@ describe('MatcherPatternPathDynamic', () => { const pattern = new MatcherPatternPathDynamic( /^\/teams\/([^/]+?)$/i, { - teamId: {}, + teamId: [{}], }, ['teams', 1] ) @@ -165,7 +165,7 @@ describe('MatcherPatternPathDynamic', () => { const pattern = new MatcherPatternPathDynamic( /^\/teams(?:\/([^/]+?))?\/b$/i, { - teamId: {}, + teamId: [{}, false, true], }, ['teams', 1, 'b'] ) @@ -180,30 +180,30 @@ describe('MatcherPatternPathDynamic', () => { expect(pattern.build({ teamId: '' })).toBe('/teams/b') }) - it.todo('optional param in the end', () => { + it('optional param in the end', () => { const pattern = new MatcherPatternPathDynamic( - /^\/teams(?:\/([^/]+?))?\/b$/i, + /^\/teams(?:\/([^/]+?))?$/i, { - teamId: {}, + teamId: [{}, false, true], }, - ['teams', 1, 'b'] + ['teams', 1] ) - expect(pattern.match('/teams/b')).toEqual({ teamId: null }) - expect(pattern.match('/teams/123/b')).toEqual({ teamId: '123' }) + expect(pattern.match('/teams')).toEqual({ teamId: null }) + expect(() => pattern.match('/teams/')).toThrow() + expect(pattern.match('/teams/123')).toEqual({ teamId: '123' }) expect(() => pattern.match('/teams/123/c')).toThrow() - expect(() => pattern.match('/teams/123/b/c')).toThrow() expect(() => pattern.match('/teams//b')).toThrow() - expect(pattern.build({ teamId: '123' })).toBe('/teams/123/b') - expect(pattern.build({ teamId: null })).toBe('/teams/b') - expect(pattern.build({ teamId: '' })).toBe('/teams/b') + expect(pattern.build({ teamId: '123' })).toBe('/teams/123') + expect(pattern.build({ teamId: null })).toBe('/teams') + expect(pattern.build({ teamId: '' })).toBe('/teams') }) it('repeatable param', () => { const pattern = new MatcherPatternPathDynamic( /^\/teams\/(.+?)\/b$/i, { - teamId: { repeat: true }, + teamId: [{}, true], }, ['teams', 1, 'b'] ) @@ -218,6 +218,25 @@ describe('MatcherPatternPathDynamic', () => { expect(pattern.build({ teamId: ['123', '456'] })).toBe('/teams/123/456/b') }) + it('repeatable param in the end', () => { + const pattern = new MatcherPatternPathDynamic( + /^\/teams\/(.+?)$/i, + { + teamId: [{}, true], + }, + ['teams', 1] + ) + + expect(pattern.match('/teams/123')).toEqual({ teamId: ['123'] }) + expect(pattern.match('/teams/123/456')).toEqual({ teamId: ['123', '456'] }) + expect(() => pattern.match('/teams')).toThrow() + expect(() => pattern.match('/teams/')).toThrow() + expect(() => pattern.match('/teams/123/')).toThrow() + expect(pattern.build({ teamId: ['123'] })).toBe('/teams/123') + expect(pattern.build({ teamId: ['123', '456'] })).toBe('/teams/123/456') + expect(() => pattern.build({ teamId: [] })).toThrow() + }) + it.todo('catch all route', () => { // const pattern = new MatcherPatternPathDynamic( }) @@ -226,9 +245,10 @@ describe('MatcherPatternPathDynamic', () => { const pattern = new MatcherPatternPathDynamic( /^\/teams\/(.*)$/i, { - pathMatch: {}, + pathMatch: [{}], }, - ['teams', 0] + ['teams', 0], + null ) expect(pattern.match('/teams/')).toEqual({ pathMatch: '' }) expect(pattern.match('/teams/123/b')).toEqual({ pathMatch: '123/b' }) @@ -245,7 +265,7 @@ describe('MatcherPatternPathDynamic', () => { const pattern = new MatcherPatternPathDynamic( /^\/teams(?:\/(.+?))?\/b$/i, { - teamId: { repeat: true }, + teamId: [{}, true, true], }, ['teams', 1, 'b'] ) @@ -268,8 +288,8 @@ describe('MatcherPatternPathDynamic', () => { const pattern = new MatcherPatternPathDynamic( /^\/teams\/([^/]+?)\/([^/]+?)$/i, { - teamId: {}, - otherId: {}, + teamId: [{}], + otherId: [{}], }, ['teams', 1, 1] ) @@ -290,8 +310,8 @@ describe('MatcherPatternPathDynamic', () => { const pattern = new MatcherPatternPathDynamic( /^\/teams\/([^/]+?)-b-([^/]+?)$/i, { - teamId: {}, - otherId: {}, + teamId: [{}], + otherId: [{}], }, ['teams', [1, '-b-', 1]] ) @@ -312,9 +332,10 @@ describe('MatcherPatternPathDynamic', () => { const pattern = new MatcherPatternPathDynamic( /^\/teams\/([^/]+?)\/$/i, { - teamId: {}, + teamId: [{}], }, - ['teams', [1, '/']] + ['teams', 1], + true ) expect(pattern.match('/teams/123/')).toEqual({ @@ -326,10 +347,11 @@ describe('MatcherPatternPathDynamic', () => { expect(pattern.build({ teamId: '123' })).toBe('/teams/123/') }) - it('can have a trailing slash after a static segment', () => { + it.todo('can have a trailing slash after a static segment', () => { const pattern = new MatcherPatternPathDynamic(/^\/teams\/b\/$/i, {}, [ 'teams', - ['b', '/'], + 'b', + ['/'], ]) expect(pattern.match('/teams/b/')).toEqual({}) @@ -343,9 +365,10 @@ describe('MatcherPatternPathDynamic', () => { const pattern = new MatcherPatternPathDynamic( /^\/teams\/(.+?)\/$/, { - teamId: { repeat: true }, + teamId: [{}, true], }, - ['teams', [1, '/']] + ['teams', 1], + true ) expect(pattern.match('/teams/123/')).toEqual({ teamId: ['123'] }) @@ -359,13 +382,14 @@ describe('MatcherPatternPathDynamic', () => { expect(pattern.build({ teamId: ['123', '456'] })).toBe('/teams/123/456/') }) - it.todo('can have a trailing slash after optional repeatable param', () => { + it('can have a trailing slash after optional repeatable param', () => { const pattern = new MatcherPatternPathDynamic( /^\/teams(?:\/(.+?))?\/$/, { - teamId: { repeat: true }, + teamId: [{}, true, true], }, - ['teams', [1, '/']] + ['teams', 1], + true ) expect(pattern.match('/teams/123/')).toEqual({ teamId: ['123'] }) diff --git a/packages/router/src/experimental/route-resolver/matchers/matcher-pattern.test-d.ts b/packages/router/src/experimental/route-resolver/matchers/matcher-pattern.test-d.ts index ac515b10..56a9a483 100644 --- a/packages/router/src/experimental/route-resolver/matchers/matcher-pattern.test-d.ts +++ b/packages/router/src/experimental/route-resolver/matchers/matcher-pattern.test-d.ts @@ -8,7 +8,7 @@ describe('MatcherPatternPathDynamic', () => { it('can be generic', () => { const matcher = new MatcherPatternPathDynamic( /^\/users\/([^/]+)$/i, - { userId: { ...PATH_PARAM_PARSER_DEFAULTS } }, + { userId: [PATH_PARAM_PARSER_DEFAULTS] }, ['users', 1] ) @@ -33,7 +33,7 @@ describe('MatcherPatternPathDynamic', () => { it('can be a simple param', () => { const matcher = new MatcherPatternPathDynamic( /^\/users\/([^/]+)\/([^/]+)$/i, - { userId: { ...PATH_PARAM_SINGLE_DEFAULT, repeat: true } }, + { userId: [PATH_PARAM_SINGLE_DEFAULT, true] }, ['users', 1] ) expectTypeOf(matcher.match('/users/123/456')).toEqualTypeOf<{ @@ -52,10 +52,10 @@ describe('MatcherPatternPathDynamic', () => { const matcher = new MatcherPatternPathDynamic( /^\/profiles\/([^/]+)$/i, { - userId: { - ...PARAM_INTEGER_SINGLE, + userId: [ + PARAM_INTEGER_SINGLE, // parser: PATH_PARAM_DEFAULT_PARSER, - }, + ], }, ['profiles', 1] ) diff --git a/packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts b/packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts index f59086ef..2363a166 100644 --- a/packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts +++ b/packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts @@ -88,19 +88,25 @@ export class MatcherPatternPathStatic /** * Options for param parsers in {@link MatcherPatternPathDynamic}. */ -export interface MatcherPatternPathDynamic_ParamOptions< +export type MatcherPatternPathDynamic_ParamOptions< TIn extends string | string[] | null = string | string[] | null, TOut = string | string[] | null, -> extends ParamParser { +> = [ + /** + * Param parser to use for this param. + */ + parser: ParamParser, + /** * Is tha param a repeatable param and should be converted to an array */ - repeat?: boolean + repeatable?: boolean, - // NOTE: not needed because in the regexp, the value is undefined if - // the group is optional and not given - // optional?: boolean -} + /** + * Can this parameter be omitted or empty (for repeatable params, an empty array). + */ + optional?: boolean, +] /** * Helper type to extract the params from the options object. @@ -115,6 +121,13 @@ type ExtractParamTypeFromOptions = { : never } +/** + * Regex to remove trailing slashes from a path. + * + * @internal + */ +const RE_TRAILING_SLASHES = /\/*$/ + /** * Handles the `path` part of a URL with dynamic parameters. */ @@ -138,12 +151,21 @@ export class MatcherPatternPathDynamic< readonly params: TParamsOptions & Record>, // 1 means a regular param, 0 means a splat, the order comes from the keys in params - readonly pathParts: Array> + readonly pathParts: Array>, + // null means "do not care", it's only for splat params + readonly trailingSlash: boolean | null = false ) { this.paramsKeys = Object.keys(this.params) as Array } match(path: string): ExtractParamTypeFromOptions { + if ( + this.trailingSlash != null && + this.trailingSlash === !path.endsWith('/') + ) { + throw miss() + } + const match = path.match(this.re) if (!match) { throw miss() @@ -152,14 +174,14 @@ export class MatcherPatternPathDynamic< for (var i = 0; i < this.paramsKeys.length; i++) { // var for performance in for loop var paramName = this.paramsKeys[i] - var paramOptions = this.params[paramName] + var [parser, repeatable] = this.params[paramName] var currentMatch = (match[i + 1] as string | undefined) ?? null - var value = paramOptions.repeat + var value = repeatable ? (currentMatch?.split('/') || []).map(decode) : decode(currentMatch) - params[paramName] = (paramOptions.get || identityFn)(value) + params[paramName] = (parser.get || identityFn)(value) } if ( @@ -177,11 +199,13 @@ export class MatcherPatternPathDynamic< build(params: ExtractParamTypeFromOptions): string { let paramIndex = 0 let paramName: keyof TParamsOptions - let paramOptions: (TParamsOptions & + let parser: (TParamsOptions & Record< string, MatcherPatternPathDynamic_ParamOptions - >)[keyof TParamsOptions] + >)[keyof TParamsOptions][0] + let repeatable: boolean | undefined + let optional: boolean | undefined let lastParamPart: number | undefined let value: ReturnType> | undefined const path = @@ -192,9 +216,13 @@ export class MatcherPatternPathDynamic< return part } else if (typeof part === 'number') { paramName = this.paramsKeys[paramIndex++] - paramOptions = this.params[paramName] + ;[parser, repeatable, optional] = this.params[paramName] lastParamPart = part - value = (paramOptions.set || identityFn)(params[paramName]) + value = (parser.set || identityFn)(params[paramName]) + + if (Array.isArray(value) && !value.length && !optional) { + throw miss() + } return Array.isArray(value) ? value.map(encodeParam).join('/') @@ -208,11 +236,11 @@ export class MatcherPatternPathDynamic< } paramName = this.paramsKeys[paramIndex++] - paramOptions = this.params[paramName] - value = (paramOptions.set || identityFn)(params[paramName]) + ;[parser, repeatable, optional] = this.params[paramName] + value = (parser.set || identityFn)(params[paramName]) // param cannot be repeatable when in a sub segment - if (__DEV__ && paramOptions.repeat) { + if (__DEV__ && repeatable) { warn( `Param "${String(paramName)}" is repeatable, but used in a sub segment of the path: "${this.pathParts.join('')}". Repeated params can only be used as a full path segment: "/file/[ids]+/something-else". This will break in production.` ) @@ -235,9 +263,9 @@ export class MatcherPatternPathDynamic< * with the original splat path: e.g. /teams/[...pathMatch] does not match /teams, so it makes * no sense to build a path it cannot match. */ - return !lastParamPart /** lastParamPart == 0 */ && !value - ? path + '/' - : path + return this.trailingSlash == null + ? path + (!value ? '/' : '') + : path.replace(RE_TRAILING_SLASHES, this.trailingSlash ? '/' : '') } } diff --git a/packages/router/src/experimental/router.spec.ts b/packages/router/src/experimental/router.spec.ts index 358e46e4..4e179f40 100644 --- a/packages/router/src/experimental/router.spec.ts +++ b/packages/router/src/experimental/router.spec.ts @@ -54,25 +54,25 @@ import { mockWarn } from '../../__tests__/vitest-mock-warn' // Create dynamic pattern matchers using the proper constructor const paramMatcher = new MatcherPatternPathDynamic( /^\/p\/([^/]+)$/, - { p: {} }, + { p: [{}] }, ['p', 1] ) const optionalMatcher = new MatcherPatternPathDynamic( /^\/optional(?:\/([^/]+))?$/, - { p: {} }, + { p: [{}] }, ['optional', 1] ) const repeatMatcher = new MatcherPatternPathDynamic( /^\/repeat\/(.+)$/, - { r: { repeat: true } }, + { r: [{}, true] }, ['repeat', 0] ) const catchAllMatcher = new MatcherPatternPathDynamic( /^\/(.*)$/, - { pathMatch: { repeat: true } }, + { pathMatch: [{}, true] }, [0] ) @@ -379,7 +379,7 @@ describe('Experimental Router', () => { it('can redirect to a star route when encoding the param', () => { const testCatchAllMatcher = new MatcherPatternPathDynamic( /^\/(.*)$/, - { pathMatch: { repeat: true } }, + { pathMatch: [{}, true] }, [0] ) const catchAllRecord = normalizeRouteRecord({ -- 2.47.3