From 1c31dce25b1edb66709b92b23ea2a7fce4b3cc60 Mon Sep 17 00:00:00 2001 From: Eduardo San Martin Morote Date: Tue, 19 Aug 2025 11:54:50 +0200 Subject: [PATCH] fix: mixed params, query and hash --- .../matchers/matcher-pattern.ts | 2 + .../route-resolver/resolver-fixed.spec.ts | 247 +++++++++++++++++- .../route-resolver/resolver-fixed.ts | 68 +++-- .../router/src/experimental/router.spec.ts | 12 +- packages/router/src/location.ts | 15 +- 5 files changed, 313 insertions(+), 31 deletions(-) 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 419418ce..128e3961 100644 --- a/packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts +++ b/packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts @@ -254,6 +254,8 @@ export interface MatcherPatternQuery< export interface MatcherPatternHash< TParams extends MatcherParamsFormatted = MatcherParamsFormatted, > extends MatcherPattern {} +// TODO: is this worth? It doesn't look like it is as it makes typing stricter but annoying +// > extends MatcherPattern<`#${string}` | '', TParams> {} /** * Generic object of params that can be passed to a matcher. diff --git a/packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts b/packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts index 1ca16b0d..187d6be9 100644 --- a/packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts +++ b/packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts @@ -1,7 +1,11 @@ import { describe, expect, it } from 'vitest' import { createFixedResolver } from './resolver-fixed' import { NO_MATCH_LOCATION } from './resolver-abstract' -import { MatcherQueryParams } from './matchers/matcher-pattern' +import { + EmptyParams, + MatcherPatternHash, + MatcherQueryParams, +} from './matchers/matcher-pattern' import { MatcherPatternQuery, MatcherPatternPathStatic, @@ -13,7 +17,7 @@ import { ANY_HASH_PATTERN_MATCHER, PAGE_QUERY_PATTERN_MATCHER, } from './matchers/test-utils' -import { miss } from './matchers/errors' +import { MatchMiss, miss } from './matchers/errors' import { MatcherPatternPath } from './matchers/matcher-pattern' // Additional pattern matchers for testing advanced scenarios @@ -67,6 +71,35 @@ const REPEATABLE_PARAM_MATCHER: MatcherPatternPath<{ p: string | string[] }> = { }, } +const OPTIONAL_NUMBER_HASH_MATCHER: MatcherPatternHash<{ + hash: number | null +}> = { + match(hash) { + if (!hash || hash === '#') return { hash: null } + const num = Number(hash.slice(1)) + if (Number.isNaN(num)) throw miss('Hash must be a number') + return { hash: num } + }, + build({ hash }) { + return hash != null ? `#${hash}` : '' + }, +} + +const OPTIONAL_NUMBER_QUERY_MATCHER: MatcherPatternQuery<{ + count: number | null +}> = { + match(query) { + if (!query.count) return { count: null } + const count = Number(query.count) + if (Number.isNaN(count)) throw miss('Count must be a number') + return { count } + }, + build({ count }) { + // return { count: count != null ? String(count) : null } + return count != null ? { count: String(count) } : ({} as EmptyParams) + }, +} + describe('fixed resolver', () => { describe('new matchers', () => { it('static path', () => { @@ -602,7 +635,7 @@ describe('fixed resolver', () => { }) }) - it('preserves empty string hash from matcher over to.hash', () => { + it('preserves hash from param even if empty', () => { const resolver = createFixedResolver([ { name: 'document', @@ -620,8 +653,8 @@ describe('fixed resolver', () => { ).toMatchObject({ name: 'document', path: '/', - params: { hash: '' }, - hash: '', // empty string from matcher is preserved + params: { hash: null }, + hash: '', fullPath: '/', }) }) @@ -654,6 +687,210 @@ describe('fixed resolver', () => { }) }) + describe('manual values breaking re-resolution', () => { + it('throws when manual hash cannot be parsed by hash matcher', () => { + const resolver = createFixedResolver([ + { + name: 'page', + path: EMPTY_PATH_PATTERN_MATCHER, + hash: OPTIONAL_NUMBER_HASH_MATCHER, + }, + ]) + + expect( + resolver.resolve({ + name: 'page', + params: {}, + hash: '#invalid-text', + }) + ).toMatchObject({ + name: 'page', + params: { hash: null }, + fullPath: '/', + hash: '', + }) + }) + + it('throws when manual query cannot be parsed by query matcher', () => { + const resolver = createFixedResolver([ + { + name: 'search', + path: EMPTY_PATH_PATTERN_MATCHER, + // this query returns {} if no count is provided as a param + // that {} gets merged with the invalid query and throws + query: [OPTIONAL_NUMBER_QUERY_MATCHER], + }, + ]) + + expect(() => + resolver.resolve({ + name: 'search', + params: {}, + query: { count: 'invalid', other: 'value' }, // Not a number + }) + ).toThrow(MatchMiss) + }) + + it('ignores the hash if a parser is provided', () => { + const resolver = createFixedResolver([ + { + name: 'page', + path: EMPTY_PATH_PATTERN_MATCHER, + hash: OPTIONAL_NUMBER_HASH_MATCHER, + }, + ]) + + expect( + resolver.resolve({ + name: 'page', + params: {}, + hash: '#42', + }) + ).toEqual({ + name: 'page', + params: { hash: null }, + fullPath: '/', + path: '/', + query: {}, + hash: '', + matched: expect.any(Array), + }) + }) + + it('succeeds and parses when manual query is valid for matcher', () => { + const resolver = createFixedResolver([ + { + name: 'search', + path: EMPTY_PATH_PATTERN_MATCHER, + query: [OPTIONAL_NUMBER_QUERY_MATCHER], + }, + ]) + + expect( + resolver.resolve({ + name: 'search', + params: {}, + query: { count: '10', other: 'value' }, // Valid number + }) + ).toEqual({ + name: 'search', + path: '/', + params: { count: 10 }, + query: { count: '10', other: 'value' }, + hash: '', + fullPath: '/?count=10&other=value', + matched: expect.any(Array), + }) + }) + + it('keeps other query values that are not params', () => { + const resolver = createFixedResolver([ + { + name: 'page', + path: EMPTY_PATH_PATTERN_MATCHER, + query: [OPTIONAL_NUMBER_QUERY_MATCHER], + hash: OPTIONAL_NUMBER_HASH_MATCHER, + }, + ]) + + expect( + resolver.resolve({ + name: 'page', + params: { hash: 42 }, + query: { count: '10', other: 'value' }, + }) + ).toEqual({ + name: 'page', + path: '/', + params: { count: 10, hash: 42 }, + query: { count: '10', other: 'value' }, + hash: '#42', + fullPath: '/?count=10&other=value#42', + matched: expect.any(Array), + }) + }) + + it('ignores manual hash if defined as param', () => { + const resolver = createFixedResolver([ + { + name: 'page', + path: EMPTY_PATH_PATTERN_MATCHER, + hash: OPTIONAL_NUMBER_HASH_MATCHER, + }, + ]) + + expect( + resolver.resolve({ + name: 'page', + params: { hash: 100 }, + hash: '#invalid', + }) + ).toMatchObject({ + name: 'page', + params: { hash: 100 }, + hash: '#100', + }) + }) + + it('preserves currentLocation hash fallback when no manual values', () => { + const resolver = createFixedResolver([ + { + name: 'page', + path: EMPTY_PATH_PATTERN_MATCHER, + hash: OPTIONAL_NUMBER_HASH_MATCHER, + }, + ]) + + const currentLocation = resolver.resolve({ + name: 'page', + params: { hash: 50 }, + }) + + // No manual values, should preserve currentLocation + expect(resolver.resolve({}, currentLocation)).toEqual({ + name: 'page', + path: '/', + params: { hash: 50 }, + hash: '#50', + fullPath: '/#50', + query: {}, + matched: expect.any(Array), + }) + }) + + it('preserves currentLocation query fallback when no manual values', () => { + const resolver = createFixedResolver([ + { + name: 'search', + path: EMPTY_PATH_PATTERN_MATCHER, + query: [OPTIONAL_NUMBER_QUERY_MATCHER], + }, + ]) + + const currentLocation = resolver.resolve({ + name: 'search', + params: { count: 20 }, + }) + + expect( + resolver.resolve( + { + query: { + other: 'value', + }, + }, + currentLocation + ) + ).toMatchObject({ + name: 'search', + path: '/', + params: { count: 20 }, + query: { count: '20', other: 'value' }, + fullPath: '/?count=20&other=value', + }) + }) + }) + describe('encoding', () => { const resolver = createFixedResolver([ { name: 'any-path', path: ANY_PATH_PATTERN_MATCHER }, diff --git a/packages/router/src/experimental/route-resolver/resolver-fixed.ts b/packages/router/src/experimental/route-resolver/resolver-fixed.ts index 0c02c1c5..aec91905 100644 --- a/packages/router/src/experimental/route-resolver/resolver-fixed.ts +++ b/packages/router/src/experimental/route-resolver/resolver-fixed.ts @@ -144,6 +144,29 @@ export function createFixedResolver< currentLocation: ResolverLocationResolved, ] + function validateMatch(record: TRecord, url: LocationNormalized) { + // match the path because the path matcher only needs to be matched here + // match the hash because only the deepest child matters + // End up by building up the matched array, (reversed so it goes from + // root to child) and then match and merge all queries + const pathParams = record.path.match(url.path) + const hashParams = record.hash?.match(url.hash) + const matched = buildMatched(record) + const queryParams: MatcherQueryParams = Object.assign( + {}, + ...matched.flatMap(record => + record.query?.map(query => query.match(url.query)) + ) + ) + // TODO: test performance + // for (const record of matched) { + // Object.assign(queryParams, record.query?.match(url.query)) + // } + + // we found our match! + return [matched, { ...pathParams, ...queryParams, ...hashParams }] as const + } + function resolve( ...[to, currentLocation]: _resolveArgs ): ResolverLocationResolved { @@ -190,14 +213,14 @@ export function createFixedResolver< } // unencoded params in a formatted form that the user came up with - const params: MatcherParamsFormatted = { + let params: MatcherParamsFormatted = { ...currentLocation?.params, ...to.params, } const path = record.path.build(params) const hash = record.hash?.build(params) ?? to.hash ?? currentLocation?.hash ?? '' - const matched = buildMatched(record) + let matched = buildMatched(record) const query = Object.assign( { ...currentLocation?.query, @@ -208,15 +231,28 @@ export function createFixedResolver< ) ) - return { - name, - fullPath: NEW_stringifyURL(stringifyQuery, path, query, hash), + const url: LocationNormalized = { + fullPath: NEW_stringifyURL( + stringifyQuery, + path, + query, + hash + ) as `/${string}`, path, - query, hash, - params, + query, + } + + // we avoid inconsistencies in params coming from query and hash + ;[matched, params] = validateMatch(record, url) + + return { + ...url, + name, matched, + params, } + // string location, e.g. '/foo', '../bar', 'baz', '?page=1' } else { // parseURL handles relative paths @@ -244,22 +280,8 @@ export function createFixedResolver< // End up by building up the matched array, (reversed so it goes from // root to child) and then match and merge all queries try { - const pathParams = record.path.match(url.path) - const hashParams = record.hash?.match(url.hash) - matched = buildMatched(record) - const queryParams: MatcherQueryParams = Object.assign( - {}, - ...matched.flatMap(record => - record.query?.map(query => query.match(url.query)) - ) - ) - // TODO: test performance - // for (const record of matched) { - // Object.assign(queryParams, record.query?.match(url.query)) - // } - - parsedParams = { ...pathParams, ...queryParams, ...hashParams } - // we found our match! + ;[matched, parsedParams] = validateMatch(record, url) + // validate throws if no match, so we should break here break } catch (e) { // for debugging tests diff --git a/packages/router/src/experimental/router.spec.ts b/packages/router/src/experimental/router.spec.ts index 0b53cf29..c97bcd0d 100644 --- a/packages/router/src/experimental/router.spec.ts +++ b/packages/router/src/experimental/router.spec.ts @@ -340,8 +340,16 @@ describe('Experimental Router', () => { it('keeps empty strings in optional params', async () => { const { router } = await newRouter() - const route1 = router.resolve({ name: 'optional', params: { p: '' } }) - expect(route1.params).toEqual({ p: '' }) + expect( + router.resolve({ name: 'optional', params: { p: '' } }) + ).toHaveProperty('params', { p: null }) + expect( + router.resolve({ name: 'optional', params: { p: null } }) + ).toHaveProperty('params', { p: null }) + expect(router.resolve({ name: 'optional', params: {} })).toHaveProperty( + 'params', + { p: null } + ) }) it('navigates to same route record but different query', async () => { diff --git a/packages/router/src/location.ts b/packages/router/src/location.ts index 73cd7b00..e32869e4 100644 --- a/packages/router/src/location.ts +++ b/packages/router/src/location.ts @@ -19,6 +19,7 @@ export interface LocationNormalized { /** * Location object accepted by {@link `stringifyURL`}. + * * @internal */ interface LocationPartial { @@ -106,7 +107,19 @@ export function parseURL( */ export function NEW_stringifyURL( stringifyQuery: (query?: LocationQueryRaw) => string, - path: LocationPartial['path'], + path: `/${string}`, + query?: LocationPartial['query'], + hash?: LocationPartial['hash'] +): `/${string}` +export function NEW_stringifyURL( + stringifyQuery: (query?: LocationQueryRaw) => string, + path: string, + query?: LocationPartial['query'], + hash?: LocationPartial['hash'] +): string +export function NEW_stringifyURL( + stringifyQuery: (query?: LocationQueryRaw) => string, + path: string, query?: LocationPartial['query'], hash: LocationPartial['hash'] = '' ): string { -- 2.47.3