Custom Route Resolvers #2415
Custom Route Resolvers #2415
Conversation
✅ Deploy Preview for vue-router canceled.
|
commit: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2415 +/- ##
==========================================
- Coverage 94.90% 89.09% -5.81%
==========================================
Files 34 45 +11
Lines 3002 4082 +1080
Branches 846 1050 +204
==========================================
+ Hits 2849 3637 +788
- Misses 150 440 +290
- Partials 3 5 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
[skip ci]
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/router/src/experimental/route-resolver/matchers/param-parsers/numbers.ts (1)
12-13: Enforce integer constraints in set() to keep get/set symmetric.
set()should mirrorget()and reject non-integers and non-finite values to avoid serializing invalid params likeNaN,Infinity, or1.5.- set: (value: number) => String(value), + set: (value: number) => { + if (!Number.isFinite(value) || !Number.isInteger(value)) throw miss() + return String(value) + },
🧹 Nitpick comments (2)
packages/router/src/experimental/route-resolver/matchers/param-parsers/numbers.ts (2)
5-11: Consider stricter integer parsing than Number() to avoid accepting '', hex, or exponential forms.
Number(value)will accept inputs like'' -> 0,'1e3' -> 1000,'0xA' -> 10, which may be surprising for URL params. If you want “decimal digits only” semantics, validate with a regex first.- get: (value: string) => { - const num = Number(value) - if (Number.isInteger(num)) { - return num - } - throw miss() - }, + get: (value: string) => { + // only allow optional leading '-' and decimal digits + if (!/^-?\d+$/.test(value)) throw miss() + // safe to parse now; result is integral by construction + return Number(value) + },If allowing exponential/hex is intended, ignore this and keep the current implementation.
4-33: Add edge-case tests to lock behavior.Ensure tests cover:
- get(): rejects '', '1.5', 'NaN', 'Infinity', and (depending on desired semantics) '1e3', '0xA'
- set(): rejects non-integers and non-finite numbers; round-trip for negative numbers and arrays
- Optional and repeatable variants: null passthrough and array mapping
I can draft these tests if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/router/src/experimental/route-resolver/matchers/param-parsers/numbers.ts(1 hunks)packages/router/src/experimental/route-resolver/resolver-fixed.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/router/src/experimental/route-resolver/resolver-fixed.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router/src/experimental/route-resolver/matchers/param-parsers/numbers.ts (2)
packages/router/src/experimental/index.ts (2)
miss(40-40)ParamParser(36-36)packages/router/src/experimental/route-resolver/matchers/param-parsers/types.ts (1)
ParamParser(7-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
packages/router/src/experimental/route-resolver/matchers/param-parsers/numbers.ts (2)
22-33: Repeatable and optional variants correctly delegate and preserve null semantics.Good composition and symmetry with SINGLE; once SINGLE.set enforces integers, these variants inherit the guarantees.
39-52: Composite PARAM_PARSER_INT dispatcher is clean and symmetric.Array/scalar/null branching is straightforward and aligns with ParamParser expectations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/router/src/experimental/route-resolver/resolver-fixed.ts (2)
185-189: Hash warning condition fix looks correct (matches prior feedback).Condition is now inverted to warn only when
hashdoes not start with#, which matches the intended behavior. Message is clear enough.
226-233: fullPath now uses the resolved absolute path for object-relative inputs (matches prior feedback).Computing
resolvedPathfirst and using it for bothurl.pathandurl.fullPathremoves the inconsistency for object-relative navigations.
🧹 Nitpick comments (4)
packages/router/src/experimental/route-resolver/resolver-fixed.ts (4)
200-208: Query precedence: route-defined query currently overrides user-provided query.Merging order places record-built query last, which can overwrite user input. Typically, framework defaults should be overridden by explicit user values. Recommend flipping the order so record-built values act as defaults, then
currentLocation.query, thento.querywins.- const query = Object.assign( - { - ...currentLocation?.query, - ...normalizeQuery(to.query), - }, - ...matched.flatMap(record => - record.query?.map(query => query.build(params)) - ) - ) + const query = Object.assign( + {}, + // record-provided defaults first + ...matched.flatMap(record => + record.query?.map(query => query.build(params)) + ), + // then existing values + currentLocation?.query, + // user-provided values last (highest precedence) + normalizeQuery(to.query) + )If the current order is intentional (record builds should trump user values), please document the precedence and add tests asserting it.
93-99: Safer parent traversal to avoid assigning null to T under an assertion.
node = node.parent as Tcan assignnullto aT-typed variable at runtime, hidden by the cast. Minor, but easy to avoid by convertingnulltoundefinedexplicitly.- let node: T | undefined = record + let node: T | undefined = record while (node) { matched.unshift(node) - node = node.parent as T + node = (node.parent || undefined) as T | undefined }
114-117: Optional: detect duplicate record names in dev.Silently overwriting in the
Mapcan be confusing. Warn in dev if a duplicate name is added.for (const record of records) { - recordMap.set(record.name, record) + if (__DEV__ && recordMap.has(record.name)) { + warn(`Duplicate resolver record name "${String(record.name)}" detected. The last one will overwrite the previous one.`) + } + recordMap.set(record.name, record) }
291-295: Avoid exposing internal array for mutation ingetRecords().Return a shallow copy to prevent external mutation of the resolver’s internal state.
return { resolve, - getRecords: () => records, + getRecords: () => records.slice(), getRecord: name => recordMap.get(name), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts(1 hunks)packages/router/src/experimental/route-resolver/resolver-fixed.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router/src/experimental/route-resolver/resolver-fixed.ts (4)
packages/router/src/experimental/route-resolver/resolver-abstract.ts (8)
RecordName(10-10)EXPERIMENTAL_Resolver_Base(19-91)ResolverLocationResolved(108-125)ResolverLocationAsPathAbsolute(196-199)ResolverLocationAsPathRelative(170-183)ResolverLocationAsNamed(147-158)ResolverLocationAsRelative(212-225)NO_MATCH_LOCATION(131-135)packages/router/src/warning.ts (1)
warn(2-9)packages/router/src/query.ts (1)
normalizeQuery(131-148)packages/router/src/location.ts (4)
NEW_stringifyURL(107-115)LocationNormalized(13-18)parseURL(43-96)resolveRelativePath(228-274)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| // unencoded params in a formatted form that the user came up with | ||
| const params: MatcherParamsFormatted = { | ||
| ...currentLocation?.params, | ||
| ...to.params, | ||
| } | ||
| const path = record.path.build(params) | ||
| const hash = record.hash?.build(params) ?? '' | ||
| const matched = buildMatched(record) |
There was a problem hiding this comment.
Object/named resolution ignores to.hash and doesn’t reuse currentLocation.hash for relative-by-name.
- Explicit
to.hashis ignored in this branch. - For relative-by-name navigations (no
to.name), the docstring says we reuse current location properties including hash, but here we always rebuild from the record.
This is a functional bug that breaks user expectations for both named and relative object navigations.
Proposed fix: respect to.hash first; for relative-by-name fallback to currentLocation.hash; otherwise use record.hash?.build(params).
const path = record.path.build(params)
- const hash = record.hash?.build(params) ?? ''
+ // Respect explicit hash, reuse current hash for relative-by-name, otherwise use record default
+ const hash =
+ to.hash ??
+ (to.name == null ? currentLocation?.hash : record.hash?.build(params)) ??
+ record.hash?.build(params) ??
+ ''
const matched = buildMatched(record)Please add/adjust tests to cover:
- Named navigation with
to.hashprovided. - Relative-by-name navigation without
to.hashprovided (should keepcurrentLocation.hash).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // unencoded params in a formatted form that the user came up with | |
| const params: MatcherParamsFormatted = { | |
| ...currentLocation?.params, | |
| ...to.params, | |
| } | |
| const path = record.path.build(params) | |
| const hash = record.hash?.build(params) ?? '' | |
| const matched = buildMatched(record) | |
| // unencoded params in a formatted form that the user came up with | |
| const params: MatcherParamsFormatted = { | |
| ...currentLocation?.params, | |
| ...to.params, | |
| } | |
| const path = record.path.build(params) | |
| // Respect explicit hash, reuse current hash for relative-by-name, otherwise use record default | |
| const hash = | |
| to.hash ?? | |
| (to.name == null ? currentLocation?.hash : record.hash?.build(params)) ?? | |
| record.hash?.build(params) ?? | |
| '' | |
| const matched = buildMatched(record) |
🤖 Prompt for AI Agents
In packages/router/src/experimental/route-resolver/resolver-fixed.ts around
lines 192-199, the object/named resolution branch currently ignores an explicit
to.hash and always rebuilds the hash from record; change the hash resolution to:
if to.hash is provided use that, else if this is a relative-by-name navigation
(no to.name and currentLocation exists) reuse currentLocation.hash, otherwise
fall back to record.hash?.build(params) ?? ''; update tests to cover a named
navigation where to.hash is provided and a relative-by-name navigation without
to.hash that should preserve currentLocation.hash.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts (4)
4-8: Use type-only imports and dedupe matcher-pattern importsThese are types-only and can be consolidated to avoid pulling types at runtime and to remove the duplicate import.
Apply:
-import { MatcherQueryParams } from './matchers/matcher-pattern' -import { - MatcherPatternQuery, - MatcherPatternPathStatic, -} from './matchers/matcher-pattern' +import type { + MatcherQueryParams, + MatcherPatternQuery, + MatcherPatternPath, +} from './matchers/matcher-pattern' +import { MatcherPatternPathStatic } from './matchers/matcher-pattern' @@ -import { MatcherPatternPath } from './matchers/matcher-pattern'Also applies to: 17-17
45-55: Optional param semantics are fine, but note the intentional “empty string” contractThe matcher returns b: '' when missing, while the type signals b?: string. This is intentional given the tests, but it slightly blurs the “optional” meaning. Consider adding a short inline comment to document that empty string represents “omitted” for this matcher so future readers don’t try to “fix” it.
210-236: Add coverage for relative resolution merging query/hash from current locationYou already cover path-relative resolution. Given resolver merges with the current location, consider adding cases where only query and/or hash are provided (or omitted) to assert the merge/override semantics for relative navigations.
Example tests to add within this describe block:
it('merges query/hash from current location when navigating relatively', () => { const resolver = createFixedResolver([{ name: 'any', path: ANY_PATH_PATTERN_MATCHER }]) const current = resolver.resolve({ path: '/base/', query: { a: '1' }, hash: '#h' }) // No new query/hash: inherit both from current expect(resolver.resolve('next', current)).toMatchObject({ path: '/base/next', query: { a: '1' }, hash: '#h', }) // Override query; keep hash expect(resolver.resolve({ path: 'next', query: { b: '2' } }, current)).toMatchObject({ path: '/base/next', query: { b: '2' }, hash: '#h', }) // Override hash; keep query expect(resolver.resolve({ path: 'next', hash: '#new' }, current)).toMatchObject({ path: '/base/next', query: { a: '1' }, hash: '#new', }) })Also applies to: 238-272
610-654: Parent/child matching: consider adding a join-path assertionGiven parent/child relationships, add one assertion that a child nested under a parent with a path prefix matches the full combined URL (e.g., /parent/child), to lock in the intended composition behavior (or lack thereof) for fixed resolvers.
Example:
it('matches child under parent prefix when applicable', () => { const parent = { name: 'parent', path: new MatcherPatternPathStatic('/users'), parent: null } const child = { name: 'detail', path: USER_ID_PATH_PATTERN_MATCHER, parent } const resolver = createFixedResolver([parent, child]) // If composition is intended: expect(resolver.resolve({ path: '/users/42' })).toMatchObject({ name: 'detail', params: { id: 42 }, matched: [parent, child], }) })If composition is explicitly not intended in fixed resolvers, adding a comment in the spec clarifying that child paths are absolute is enough.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts (4)
packages/router/src/experimental/index.ts (4)
MatcherPatternPath(26-26)miss(40-40)createFixedResolver(16-16)MatcherPatternQuery(27-27)packages/router/src/experimental/route-resolver/resolver-fixed.ts (1)
createFixedResolver(110-296)packages/router/src/experimental/route-resolver/matchers/test-utils.ts (5)
USER_ID_PATH_PATTERN_MATCHER(30-46)EMPTY_PATH_PATTERN_MATCHER(20-28)ANY_PATH_PATTERN_MATCHER(9-18)PAGE_QUERY_PATTERN_MATCHER(48-57)ANY_HASH_PATTERN_MATCHER(59-63)packages/router/src/experimental/route-resolver/resolver-abstract.ts (1)
NO_MATCH_LOCATION(131-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts (3)
19-68: Solid custom matchers for exercising complex scenariosThe additional path matchers (USERS_ID_OTHER_PATH_MATCHER, AB_PARAMS_PATH_MATCHER, AB_OPTIONAL_PATH_MATCHER, REPEATABLE_PARAM_MATCHER) are clear and cover important shapes (multi-segment, optional, repeatable). Good foundation for the suite.
126-136: NO_MATCH_LOCATION equality is precise; keep as isLeveraging the exported NO_MATCH_LOCATION for deep equality (including the Symbol name) is a good way to avoid brittle expectations. Nice.
407-477: Encoding/decoding assertions are thoroughNice coverage for path, query, and hash encode/decode symmetry, including passing decoded query to matchers and ensuring fullPath encodes while keeping normalized properties decoded.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (4)
32-41: JSDoc param name for build() is misleadingThe build() doc still references "@param value" even though the parameter is named "params" and is used for serialization, not parsing.
Apply this diff to align the JSDoc with the actual API:
/** * Build a serializable value from parsed params. Should apply encoding if the * returned value is a string (e.g path and hash should be encoded but query * shouldn't). * - * @param value - params value to parse + * @param params - params to serialize * @returns serialized params value */ build(params: TOut): TIn
96-99: Typo in param option description ("tha" → "the")Minor wording fix to avoid leaking typos in public types.
/** - * Is tha param a repeatable param and should be converted to an array + * Is the param repeatable and should it be converted to an array */ repeat?: boolean
165-172: Dev warning uses inaccurate counts; compute matched groups vs defined params
match.lengthincludes the full match at index 0. The message also relies oni(loop index) instead of the intended param count. Use explicit counts for clarity and correctness.- if ( - __DEV__ && - Object.keys(params).length !== Object.keys(this.params).length - ) { - warn( - `Regexp matched ${match.length} params, but ${i} params are defined. Found when matching "${path}" against ${String(this.re)}` - ) - } + if (__DEV__) { + const matchedGroups = match.length - 1 + const definedParams = this.paramsKeys.length + if (matchedGroups !== definedParams) { + warn( + `Regexp captured ${matchedGroups} group(s), but ${definedParams} param(s) are defined. Found when matching "${path}" against ${String( + this.re + )}` + ) + } + }
232-239: Append trailing slash only if the last path part is a splatAs written,
lastParamPart && !valuecould incorrectly append a trailing slash if a splat param appears before the end of the path. Restrict the behavior to when the last path part is a splat.- return lastParamPart && !value ? path + '/' : path + const lastPart = this.pathParts[this.pathParts.length - 1] + return lastPart === 1 && !value ? path + '/' : pathAlso fix a small typo in the comment.
/** * If the last part of the path is a splat param and its value is empty, it gets - * filteretd out, resulting in a path that doesn't end with a `/` and doesn't even match + * filtered out, resulting in a path that doesn't end with a `/` and doesn't even match * 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. */packages/router/src/experimental/router.spec.ts (1)
101-146: Nit: avoid duplicating parent instances in test setup
childRawRecord.parentpoints to a normalized parent (parentRecord) whilerouteRecordsalso includesparentRawRecord, which gets normalized again. This creates two parent instances. It’s harmless here but can make parent traversals/assertions harder to reason about.If convenient, consider either:
- Referencing only
parentRawRecordinrouteRecordsand wiringchildRawRecord.parentafter normalization, or- Reusing
parentRecordconsistently without re-normalizing the raw parent again when buildingexperimentalRoutes.packages/router/src/experimental/router.ts (3)
617-619: Defensive check before touching history.scrollRestorationIn some embedded/non-standard environments,
scrollRestorationmight be missing or non-writable. Guard the assignment.- if (isBrowser && options.scrollBehavior) { - history.scrollRestoration = 'manual' - } + if (isBrowser && options.scrollBehavior && 'scrollRestoration' in history) { + try { + history.scrollRestoration = 'manual' + } catch { + // ignore if not writable + } + }
712-714: Dev warning for no match: include a safe stringified representation of 'to'
"${to}"will print[object Object]whentois an object. Prefer JSON.stringify for clarity.- if (!matchedRoute.matched.length) { - warn(`No match found for location with path "${to}"`) - } + if (!matchedRoute.matched.length) { + const toStr = + typeof to === 'string' ? to : JSON.stringify(to, null, 0) + warn(`No match found for location "${toStr}"`) + }
1388-1394: Remove @ts-expect-error on $router assignment by augmenting Vue typesRather than suppressing, expose the experimental router type via module augmentation so
$routeris correctly typed.Below is a minimal augmentation you can add to packages/router/src/experimental/index.ts (outside of this file):
// packages/router/src/experimental/index.ts import type { EXPERIMENTAL_Router } from './router' declare module 'vue' { interface ComponentCustomProperties { $router: EXPERIMENTAL_Router // Optionally specialize $route if the experimental normalized type differs // $route: RouteLocationNormalizedLoaded } }Then, remove the
@ts-expect-errorhere. This keeps the public API intentional and type-safe.I can draft the exact augmentation and adjust any affected tests/usages if you’d like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/router/src/experimental/index.ts(1 hunks)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts(1 hunks)packages/router/src/experimental/router.spec.ts(1 hunks)packages/router/src/experimental/router.ts(1 hunks)packages/router/vitest.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/router/vitest.config.ts
- packages/router/src/experimental/index.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-14T13:58:50.446Z
Learnt from: posva
PR: vuejs/router#2415
File: packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts:0-0
Timestamp: 2025-08-14T13:58:50.446Z
Learning: The `encodeParam` function in vue-router handles nullish values (null/undefined) by returning an empty string, rather than stringifying them to "null" or "undefined". This means missing parameters in route building result in empty path segments rather than literal "undefined" in URLs.
Applied to files:
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts
📚 Learning: 2025-08-14T13:58:50.446Z
Learnt from: posva
PR: vuejs/router#2415
File: packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts:0-0
Timestamp: 2025-08-14T13:58:50.446Z
Learning: The `encodeParam` function in vue-router has the signature `(text: string | number | null | undefined): string` and specifically handles nullish values by returning an empty string via `text == null ? '' : encodePath(text).replace(SLASH_RE, '%2F')`. This means missing or null parameters in route building result in empty path segments rather than literal "null"/"undefined" strings in URLs.
Applied to files:
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts
📚 Learning: 2025-08-14T13:59:53.367Z
Learnt from: posva
PR: vuejs/router#2415
File: packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts:127-129
Timestamp: 2025-08-14T13:59:53.367Z
Learning: NoInfer<T> is a native TypeScript utility type introduced in TypeScript 5.4 (March 2024) that prevents type inference in generic contexts. It does not need to be defined or imported in TypeScript projects using version 5.4 or later.
Applied to files:
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts
📚 Learning: 2025-08-14T13:59:53.367Z
Learnt from: posva
PR: vuejs/router#2415
File: packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts:127-129
Timestamp: 2025-08-14T13:59:53.367Z
Learning: NoInfer<T> is a native TypeScript utility type introduced in TypeScript 5.4 that prevents type inference in generic contexts. It does not need to be defined or imported in TypeScript projects using version 5.4 or later.
Applied to files:
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts
🧬 Code Graph Analysis (3)
packages/router/src/experimental/router.spec.ts (3)
packages/router/__tests__/utils.ts (4)
components(127-168)newRouter(170-177)createDom(99-121)tick(21-25)packages/router/__tests__/vitest-mock-warn.ts (1)
mockWarn(5-114)packages/router/src/index.ts (1)
NavigationFailureType(144-144)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (6)
packages/router/src/experimental/route-resolver/matchers/param-parsers/types.ts (1)
ParamParser(7-13)packages/router/src/experimental/route-resolver/matchers/test-utils.ts (2)
match(12-14)match(32-42)packages/router/src/experimental/route-resolver/matchers/matcher-pattern-path-star.ts (1)
match(26-33)packages/router/src/encoding.ts (3)
decode(147-157)encodeParam(133-135)encodePath(120-122)packages/router/src/utils/index.ts (1)
identityFn(15-15)packages/router/src/warning.ts (1)
warn(2-9)
packages/router/src/experimental/router.ts (9)
packages/router/src/experimental/route-resolver/resolver-fixed.ts (4)
EXPERIMENTAL_ResolverRecord_Base(27-53)EXPERIMENTAL_ResolverRecord_Matchable(71-75)EXPERIMENTAL_ResolverRecord_Group(58-66)EXPERIMENTAL_ResolverFixed(84-85)packages/router/src/types/index.ts (1)
RouteMeta(260-260)packages/router/src/errors.ts (3)
NavigationFailure(58-74)NavigationRedirectError(81-85)isNavigationFailure(184-193)packages/router/src/location.ts (3)
START_LOCATION_NORMALIZED(291-302)parseURL(43-96)isSameRouteLocation(153-169)packages/router/src/utils/env.ts (1)
isBrowser(1-1)packages/router/src/query.ts (2)
parseQuery(54-81)stringifyQuery(92-121)packages/router/src/experimental/route-resolver/resolver-abstract.ts (4)
ResolverLocationResolved(108-125)ResolverLocationAsPathRelative(170-183)ResolverLocationAsNamed(147-158)ResolverLocationAsRelative(212-225)packages/router/src/utils/index.ts (3)
assign(46-46)isArray(68-69)noop(62-62)packages/router/src/navigationGuards.ts (3)
extractChangingRecords(417-447)extractComponentsGuards(234-354)guardToPromiseFn(123-213)
🪛 GitHub Actions: test
packages/router/src/experimental/router.spec.ts
[error] 46-46: tsc --build tsconfig.json failed during 'pnpm run -r test:types': TypeScript error TS6133: 'loadRouteLocation' is declared but its value is never read.
🔇 Additional comments (3)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (1)
63-85: Case-insensitive static matcher preserves original casing on build — LGTMConstructor stores original path and lowercased version for matching; build() returns original casing. This aligns with previous feedback and avoids mutating consumer-supplied paths.
packages/router/src/experimental/router.spec.ts (1)
279-292: Hash history resolution assertions look correct — LGTMThe expected
hrefformat for hash history and base is consistent with Vue Router behavior.packages/router/src/experimental/router.ts (1)
891-905: Guard-redirect branch uses failure.to correctly — LGTMThe redirect flow now resolves
failure.to(notshouldRedirect) and preservesreplace,state, andredirectedFrom. That fixes the earlier bug risk.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/router/src/experimental/router.spec.ts (3)
233-260: Test title doesn’t match implementation (“component-less” vs havingcomponents)The test description says “component-less route records” but both records define
components. Either adjust the title or dropcomponentsif you intend to assert meta merging for component-less records.If you prefer adjusting the title:
- it('merges meta properties from component-less route records', async () => { + it('merges meta properties from hierarchical records', async () => {If you intended component-less records, remove
componentsfrom both records.
151-159: TightennewRouter()option typing: avoidanyforresolverUsing
anyloses type-safety for the experimental API in this suite. You can derive the resolver type fromcreateFixedResolverto retain checks.- options: Partial<Omit<EXPERIMENTAL_RouterOptions, 'resolver'>> & { - resolver?: any - } = {} + options: Partial<Omit<EXPERIMENTAL_RouterOptions, 'resolver'>> & { + resolver?: ReturnType<typeof createFixedResolver> + } = {}
295-301: Nit: fix grammar in test names (“can replaces” → “can replace”)Minor wording fix to keep test descriptions clean.
-it('can replaces current location with a string location', async () => { +it('can replace current location with a string location', async () => {-it('can replaces current location with an object location', async () => { +it('can replace current location with an object location', async () => {Also applies to: 303-310
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/router/src/experimental/router.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: posva
PR: vuejs/router#2415
File: packages/router/src/experimental/index.ts:43-44
Timestamp: 2025-08-11T15:22:32.526Z
Learning: In the Vue Router codebase, files within the src directory should not import from src/index to avoid circular dependencies. Deep imports like `../matcher/types` are intentional and necessary for maintaining proper dependency hierarchy.
🧬 Code Graph Analysis (1)
packages/router/src/experimental/router.spec.ts (1)
packages/router/__tests__/utils.ts (2)
components(127-168)tick(21-25)
🪛 GitHub Actions: test
packages/router/src/experimental/router.spec.ts
[error] 235-235: Step 'packages/router test:types' failed: TypeScript error TS2769: No overload matches this call. (src/experimental/router.spec.ts:235:27)
🔇 Additional comments (3)
packages/router/src/experimental/router.spec.ts (3)
171-176: LGTM: start location assertion for experimental routerSanity check against
START_LOCATION_NORMALIZEDlooks correct for the experimental setup.
215-221: LGTM:stringifyQueryis not invoked byresolvewithout queryThe assertion correctly verifies that
stringifyQueryisn’t called when resolving without a query. Good guard for the experimental resolver behavior.
345-351: LGTM: re-navigation to same record with different query/hashThese cover important semantics for “same record, different search/hash” and match expected behavior.
Also applies to: 353-359
| import fakePromise from 'faked-promise' | ||
| import { | ||
| experimental_createRouter, | ||
| createFixedResolver, | ||
| MatcherPatternPathStatic, | ||
| MatcherPatternPathDynamic, | ||
| EXPERIMENTAL_RouteRecord_Matchable, | ||
| EXPERIMENTAL_RouterOptions, | ||
| normalizeRouteRecord, | ||
| } from './index' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid importing from src/index inside src to prevent circular deps
Per established codebase conventions (and past cycles), importing from ../index within src can create circular dependencies. Prefer deep imports (e.g., ../history/memory, ../history/html5, ../history/hash, and typing imports from the concrete module exporting the types).
Would you like me to generate a PR-ready diff switching these to deep imports once we confirm the exact module paths for the exported types?
You can scan for risky src/index imports with:
Also applies to: 42-47
🏁 Script executed:
#!/bin/bash
# Find index imports inside src that could create cycles
rg -n -C2 -P "from ['\"]\.\./index['\"]|from ['\"]\./index['\"]" -- packages/router/srcLength of output: 893
🏁 Script executed:
#!/bin/bash
# Locate the definition of RouteLocationRaw
rg -n "export type RouteLocationRaw" -g "packages/router/src/**/*.ts"Length of output: 203
🏁 Script executed:
#!/bin/bash
# Locate experimental exports in src/experimental
rg -n "export .*experimental_createRouter" -g "packages/router/src/experimental/*.ts"
rg -n "export .*createFixedResolver" -g "packages/router/src/experimental/*.ts"
rg -n "export .*normalizeRouteRecord" -g "packages/router/src/experimental/*.ts"
rg -n "export .*EXPERIMENTAL_RouteRecord_Matchable" -g "packages/router/src/experimental/*.ts"
rg -n "export .*EXPERIMENTAL_RouterOptions" -g "packages/router/src/experimental/*.ts"
# Locate matcher type definitions
rg -n "export type MatcherPatternPathStatic" -g "packages/router/src/matcher/types.ts"
rg -n "export type MatcherPatternPathDynamic" -g "packages/router/src/matcher/types.ts"
# Locate history function definitions
rg -n "export function createMemoryHistory" -g "packages/router/src/history/*.ts"
rg -n "export function createWebHistory" -g "packages/router/src/history/*.ts"
rg -n "export function createWebHashHistory" -g "packages/router/src/history/*.ts"Length of output: 2202
🏁 Script executed:
#!/bin/bash
# Locate the definitions of MatcherPatternPathStatic and MatcherPatternPathDynamic across the codebase
rg -n "MatcherPatternPathStatic" -- packages/router/src
rg -n "MatcherPatternPathDynamic" -- packages/router/srcLength of output: 7306
Refactor tests to use deep imports instead of index
Tests in src/experimental should import directly from their source modules to avoid circular dependencies, per codebase conventions.
• File needing changes:
packages/router/src/experimental/router.spec.ts(lines 32–47)
Apply this diff:
- import {
- experimental_createRouter,
- createFixedResolver,
- MatcherPatternPathStatic,
- MatcherPatternPathDynamic,
- EXPERIMENTAL_RouteRecord_Matchable,
- EXPERIMENTAL_RouterOptions,
- normalizeRouteRecord,
- } from './index'
+ import {
+ experimental_createRouter,
+ normalizeRouteRecord,
+ EXPERIMENTAL_RouteRecord_Matchable,
+ EXPERIMENTAL_RouterOptions,
+ } from './router'
+ import { createFixedResolver } from './route-resolver/resolver-fixed'
+ import {
+ MatcherPatternPathStatic,
+ MatcherPatternPathDynamic,
+ } from './route-resolver/matchers/matcher-pattern'
- import {
- createMemoryHistory,
- createWebHashHistory,
- RouteLocationRaw,
- } from '../index'
+ import { createMemoryHistory } from '../history/memory'
+ import { createWebHashHistory } from '../history/hash'
+ import { RouteLocationRaw } from '../typed-routes/route-location'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import fakePromise from 'faked-promise' | |
| import { | |
| experimental_createRouter, | |
| createFixedResolver, | |
| MatcherPatternPathStatic, | |
| MatcherPatternPathDynamic, | |
| EXPERIMENTAL_RouteRecord_Matchable, | |
| EXPERIMENTAL_RouterOptions, | |
| normalizeRouteRecord, | |
| } from './index' | |
| import fakePromise from 'faked-promise' | |
| import { | |
| experimental_createRouter, | |
| normalizeRouteRecord, | |
| EXPERIMENTAL_RouteRecord_Matchable, | |
| EXPERIMENTAL_RouterOptions, | |
| } from './router' | |
| import { createFixedResolver } from './route-resolver/resolver-fixed' | |
| import { | |
| MatcherPatternPathStatic, | |
| MatcherPatternPathDynamic, | |
| } from './route-resolver/matchers/matcher-pattern' | |
| import { createMemoryHistory } from '../history/memory' | |
| import { createWebHashHistory } from '../history/hash' | |
| import { RouteLocationRaw } from '../typed-routes/route-location' |
🤖 Prompt for AI Agents
In packages/router/src/experimental/router.spec.ts around lines 32–41, the test
is importing many symbols from './index' which can create circular deps; replace
that single index import with direct (deep) imports from the modules that
actually export each symbol (import experimental_createRouter from the router
implementation file, createFixedResolver from its resolver module,
MatcherPatternPathStatic and MatcherPatternPathDynamic from their matcher
modules, EXPERIMENTAL_RouteRecord_Matchable and EXPERIMENTAL_RouterOptions from
their types modules, and normalizeRouteRecord from its normalize module) so each
named export is imported from its source file path under
packages/router/src/experimental (update the import lines accordingly).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/router/src/experimental/route-resolver/resolver-fixed.ts (3)
185-189: LGTM: hash format warning condition/message fixed.The condition now correctly warns only when the hash does not start with “#”, and the message suggests the proper replacement.
227-234: LGTM:fullPathnow uses the resolved absolute path for object-relative inputs.
resolvedPathis computed and used both forurl.pathand when buildingfullPath, ensuring consistency with the string-path branch.
198-200: Fix hash precedence in named/relative resolution (respect explicitto.hash, keep current hash for relative-by-name).Explicit
to.hashshould take precedence over defaults. For relative-by-name navigations (noto.name), the current location’s hash should be preserved whento.hashis not provided. The current implementation prefersrecord.hash?.build(params), which overrides explicit inputs and breaks expected behavior.Apply this diff to correct the precedence:
- const path = record.path.build(params) - const hash = - record.hash?.build(params) ?? to.hash ?? currentLocation?.hash ?? '' + const path = record.path.build(params) + // Respect explicit hash; for relative-by-name, reuse current hash; otherwise use record default. + const isRelativeByName = to.name == null + const hash = + to.hash ?? + (isRelativeByName ? currentLocation?.hash : undefined) ?? + record.hash?.build(params) ?? + ''Please add tests covering:
- Named navigation with
to.hashprovided (should useto.hash).- Relative-by-name navigation without
to.hash(should reusecurrentLocation.hash).
🧹 Nitpick comments (4)
packages/router/src/experimental/route-resolver/resolver-fixed.ts (4)
154-160: Nit: missing space in DEV warning message.The error string concatenation misses a space before “without”.
Apply this diff:
- warn( - `Cannot resolve relative location "${JSON.stringify(to)}"without a current location. This will throw in production.`, - to - ) + warn( + `Cannot resolve relative location "${JSON.stringify(to)}" without a current location. This will throw in production.`, + to + )
227-234: Optional: unify hash-format warning for object-path locations too.For consistency with the named branch, warn in DEV when
to.hashis provided without a leading “#”.Apply this diff:
} else { const query = normalizeQuery(to.query) const path = resolveRelativePath(to.path, currentLocation?.path || '/') + if (__DEV__ && to.hash && !to.hash.startsWith('#')) { + warn( + `A \`hash\` should start with "#". Replace "${to.hash}" with "#${to.hash}".` + ) + } url = { fullPath: NEW_stringifyURL(stringifyQuery, path, query, to.hash), path, query, hash: to.hash || '', } }
93-99: Optional: guard against cyclic parent chains inbuildMatched.A malformed graph could cause an infinite loop. Cheap DEV-only guard improves robustness without affecting prod performance.
Apply this diff:
const matched: T[] = [] - let node: T | undefined = record - while (node) { - matched.unshift(node) - node = node.parent as T - } + let node: T | undefined = record + if (__DEV__) { + const seen = new Set<T>() + while (node && !seen.has(node)) { + seen.add(node) + matched.unshift(node) + node = node.parent as T + } + if (node) { + warn('Detected cyclic parent chain in route records. Stopping at first repeated record.') + } + } else { + while (node) { + matched.unshift(node) + node = node.parent as T + } + }
115-117: Optional: warn on duplicate record names when building the map.Silently overwriting can be surprising. Emitting a DEV warning helps catch misconfigurations early.
Apply this diff:
for (const record of records) { - recordMap.set(record.name, record) + if (__DEV__ && recordMap.has(record.name)) { + warn( + `Duplicate record name "${String(record.name)}" found. The last one will override the previous definition.` + ) + } + recordMap.set(record.name, record) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts(1 hunks)packages/router/src/experimental/route-resolver/resolver-fixed.ts(1 hunks)packages/router/src/experimental/router.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/router/src/experimental/router.spec.ts
- packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router/src/experimental/route-resolver/resolver-fixed.ts (4)
packages/router/src/experimental/route-resolver/resolver-abstract.ts (8)
RecordName(10-10)EXPERIMENTAL_Resolver_Base(19-91)ResolverLocationResolved(108-125)ResolverLocationAsPathAbsolute(196-199)ResolverLocationAsPathRelative(170-183)ResolverLocationAsNamed(147-158)ResolverLocationAsRelative(212-225)NO_MATCH_LOCATION(131-135)packages/router/src/warning.ts (1)
warn(2-9)packages/router/src/query.ts (1)
normalizeQuery(131-148)packages/router/src/location.ts (4)
NEW_stringifyURL(107-115)LocationNormalized(13-18)parseURL(43-96)resolveRelativePath(228-274)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
packages/router/src/experimental/route-resolver/resolver-fixed.ts (1)
200-209: Confirm query precedence: record-built query vs user-providedto.query.Current order makes record-built query entries override
currentLocation.queryandto.querywhen keys collide (because record-built entries are assigned last). If the intent is for explicit user input to win, swap the order.If you want
to.queryto override record defaults, change to:- const query = Object.assign( - { - ...currentLocation?.query, - ...normalizeQuery(to.query), - }, - ...matched.flatMap(record => - record.query?.map(query => query.build(params)) - ) - ) + const query = Object.assign( + {}, + ...matched.flatMap(record => + record.query?.map(query => query.build(params)) + ), + currentLocation?.query, + normalizeQuery(to.query) + )Please confirm intended precedence before applying.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/router/src/location.ts (1)
9-13: Remove stale@internaltag on exported interface
LocationNormalizedis exported but still marked@internalin the JSDoc./** * Location object returned by {@link `parseURL`}. - * @internal */ export interface LocationNormalized {packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts (1)
487-493: Guard missing-named-route test by environment (__DEV__)The resolver only throws in DEV; in production it should return a no-match location. Make the assertion environment-aware to avoid flakes.
it('throws if named route does not exist', () => { const resolver = createFixedResolver([]) - expect(() => - resolver.resolve({ name: 'nonexistent', params: {} }) - ).toThrowError('Record "nonexistent" not found') + if (__DEV__) { + expect(() => + resolver.resolve({ name: 'nonexistent', params: {} }) + ).toThrowError('Record "nonexistent" not found') + } else { + expect( + resolver.resolve({ name: 'nonexistent', params: {} }) + ).toMatchObject(NO_MATCH_LOCATION) + } })
🧹 Nitpick comments (7)
packages/router/src/location.ts (2)
74-79: Minor readability: consider??=for path fallbackSmall readability tweak: using nullish coalescing assignment makes the “only set when not already set” intention explicit.
- // TODO(major): path ||= - path = path || location.slice(0, hashPos) + // TODO(major): path ||= + path ??= location.slice(0, hashPos)
99-129: LGTM: PublicNEW_stringifyURLhelper matches parse/encode contract
- Encodes the hash via
encodeHashand preserves query building via injectedstringifyQuery.- Overloads allow stricter typing for absolute paths.
Two small nits to consider:
- The “NEW_” prefix likely isn’t final. When ready, rename to a stable name (e.g.,
stringifyURLParts) and deprecate the oldstringifyURLto avoid confusion.- Ensure
encodeHashconsistently preserves a leading#(as tests imply) and encodes the rest—current behavior appears correct given the spec tests.packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts (1)
691-712: Rename test: it doesn’t throw for invalid manual hashThe test title says “throws” but the expectation asserts a successful resolution that ignores the manual hash when a hash matcher is provided. Rename to avoid confusion.
-it('throws when manual hash cannot be parsed by hash matcher', () => { +it('ignores manual hash when a hash matcher is defined and the value is invalid', () => {packages/router/src/experimental/route-resolver/resolver-fixed.ts (4)
178-182: Fix typo: missing space in warning messageThere is a missing space before “without”. Minor text polish.
- `Cannot resolve relative location "${JSON.stringify(to)}"without a current location. This will throw in production.`, + `Cannot resolve relative location "${JSON.stringify(to)}" without a current location. This will throw in production.`,
200-206: Confirm intended prod behavior for missing named records; consider graceful NO_MATCH fallbackIn DEV you throw if the named record is missing. In production,
recordMap.get(name)!can beundefined, and subsequent access (record.path.build(...)) would crash. If the intended behavior in prod is to return a no-match location (as hinted in prior discussions), add a non-DEV fallback.Proposed update:
- const record = recordMap.get(name)! - - if (__DEV__) { - if (!record || !name) { - throw new Error(`Record "${String(name)}" not found`) - } + const record = recordMap.get(name) + + if (!record || !name) { + if (__DEV__) { + throw new Error(`Record "${String(name)}" not found`) + } else { + const query = normalizeQuery((to as ResolverLocationAsNamed).query) + const hash = + (to as ResolverLocationAsNamed).hash ?? currentLocation?.hash ?? '' + const path = currentLocation?.path ?? '/' + return { + ...NO_MATCH_LOCATION, + fullPath: NEW_stringifyURL(stringifyQuery, path, query, hash), + path, + query, + hash, + } as any + } + } + if (__DEV__) { if (typeof to === 'object' && to.hash && !to.hash.startsWith('#')) { warn( `A \`hash\` should always start with the character "#". Replace "${to.hash}" with "#${to.hash}".` ) } }If returning NO_MATCH in prod isn’t desired, please confirm; I’ll adjust the spec accordingly.
257-271: Consider dev warning for invalid hash format in object-path branchYou already warn for named locations when
to.hashdoesn’t start with#. For consistency, add the same DEV-only warning in the object-relative/object-absolute branch.} else { const query = normalizeQuery(to.query) const path = resolveRelativePath(to.path, currentLocation?.path || '/') + if (__DEV__ && to.hash && !to.hash.startsWith('#')) { + warn( + `A \`hash\` should always start with the character "#". Replace "${to.hash}" with "#${to.hash}".` + ) + } url = { fullPath: NEW_stringifyURL(stringifyQuery, path, query, to.hash), path, query, hash: to.hash || '', } }
147-168: Efficient param aggregation; watch exception-driven control flow in large setsMerging query params via
Object.assign({}, ...flatMap(...match))is clean. The exception-driven “first match wins” loop is fine for small-to-medium sets; if record counts become large, consider explicit “canMatch” checks to avoid exception cost on hot paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.gitignore(1 hunks)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts(1 hunks)packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts(1 hunks)packages/router/src/experimental/route-resolver/resolver-fixed.ts(1 hunks)packages/router/src/experimental/router.spec.ts(1 hunks)packages/router/src/location.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .gitignore
- packages/router/src/experimental/router.spec.ts
- packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/router/src/experimental/route-resolver/resolver-fixed.ts (5)
packages/router/src/experimental/route-resolver/resolver-abstract.ts (8)
RecordName(10-10)EXPERIMENTAL_Resolver_Base(19-91)ResolverLocationResolved(108-125)ResolverLocationAsPathAbsolute(196-199)ResolverLocationAsPathRelative(170-183)ResolverLocationAsNamed(147-158)ResolverLocationAsRelative(212-225)NO_MATCH_LOCATION(131-135)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (5)
MatcherPatternPath(47-50)MatcherPatternQuery(246-248)MatcherPatternHash(254-256)MatcherQueryParams(275-275)MatcherParamsFormatted(263-263)packages/router/src/location.ts (4)
LocationNormalized(13-18)NEW_stringifyURL(120-128)parseURL(44-97)resolveRelativePath(241-287)packages/router/src/warning.ts (1)
warn(2-9)packages/router/src/query.ts (1)
normalizeQuery(131-148)
packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts (4)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (8)
MatcherPatternPath(47-50)match(76-81)match(146-175)MatcherPatternHash(254-256)MatcherPatternQuery(246-248)EmptyParams(268-268)MatcherPatternPathStatic(63-86)MatcherQueryParams(275-275)packages/router/src/experimental/route-resolver/resolver-fixed.ts (1)
createFixedResolver(110-319)packages/router/src/experimental/route-resolver/matchers/test-utils.ts (5)
USER_ID_PATH_PATTERN_MATCHER(30-46)EMPTY_PATH_PATTERN_MATCHER(20-28)ANY_PATH_PATTERN_MATCHER(9-18)PAGE_QUERY_PATTERN_MATCHER(48-57)ANY_HASH_PATTERN_MATCHER(59-63)packages/router/src/experimental/route-resolver/resolver-abstract.ts (1)
NO_MATCH_LOCATION(131-135)
packages/router/src/location.ts (2)
packages/router/src/query.ts (3)
parseQuery(54-81)stringifyQuery(92-121)LocationQueryRaw(40-43)packages/router/src/encoding.ts (2)
decode(147-157)encodeHash(76-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
packages/router/src/location.ts (4)
54-61: LGTM: Correctly disambiguates?within#fragmentsThe guard
searchPos = hashPos >= 0 && searchPos > hashPos ? -1 : searchPosprevents treating?inside a hash as a query. This aligns with URL semantics and avoids accidental query parsing for cases like/foo#hash?query.
62-70: LGTM: Preserve leading?insearchStringKeeping the
?makes concatenation intofullPathstraightforward and stays compatible withparseQuerywhich handles both'?a=1'and'a=1'.
81-88: LGTM: Relative resolution and empty-to behavior
resolveRelativePath(path != null ? path : location, currentLocation)plus the explicit empty-to behavior avoids oddities when parsing'?q=1'or'#h'with a base.
250-256: LGTM: Explicit typing and empty-to preservation
- Early-return on empty
tomatches the intended semantics (resolve''tofrom).- Explicit
lastToSegment: string | undefinedimproves clarity and tooling.packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts (2)
438-449: LGTM: Object path treated as raw pathnameGood explicit coverage to ensure object location
{ path: '/?a=a&b=b#h' }treats the entire string as a pathname (no query/hash parsing), matching the documented behavior for object-style inputs.
948-964: LGTM: Encoding behavior validatedThe tests for encoding query values and hash fragments (e.g.,
"to%22) confirmNEW_stringifyURL+encodeHashcontracts. This prevents regressions in URL construction logic.packages/router/src/experimental/route-resolver/resolver-fixed.ts (1)
221-233: Hash/Query precedence logic matches tests
- Hash: matcher-built hash overrides
to.hashandcurrentLocation.hash(as tests assert).- Query: matcher-built query overrides
to.query, while non-param keys are preserved.No change requested; just calling out the alignment with the spec file.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router/src/router.ts (1)
772-780: Ensure shouldRedirect is always an object before merging optionsassign(shouldRedirect, …) mutates/merges into a primitive when shouldRedirect is a string. Wrap with locationAsObject() like in pushWithRedirect() to normalize.
- pushWithRedirect( - assign(shouldRedirect, { replace: true, force: true }), + pushWithRedirect( + assign(locationAsObject(shouldRedirect), { + replace: true, + force: true, + }), toLocation ).catch(noop)
♻️ Duplicate comments (4)
packages/router/src/experimental/router.ts (2)
1344-1349: Global properties typing: add augmentation or cast — don’t ship@ts-expect-errorThis suppression stems from $router being typed as the stable Router. Augment Vue’s ComponentCustomProperties in the experimental surface or perform a clearly-documented cast here.
Proposed augmentation (in packages/router/src/experimental/index.ts):
// packages/router/src/experimental/index.ts import type { EXPERIMENTAL_Router } from './router' declare module 'vue' { export interface ComponentCustomProperties { $router: EXPERIMENTAL_Router // add $route here too if the experimental route type differs } }Alternatively, use a constrained cast at the injection site:
type RouterAdapter = { $router: EXPERIMENTAL_Router } & Record<string, unknown> // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access ;(app.config.globalProperties as unknown as RouterAdapter).$router = router
662-714: Track and eliminate remaining @ts-expect-error occurrences in experimental routerThere are still multiple suppressions around resolve(), redirect handling, and API exposure (resolve/push/replace). The targeted casts above remove several. The remaining ones should be replaced with precise types (e.g., unify EXPERIMENTAL_Router_Base method signatures with the implemented functions).
Run this to audit remaining suppressions and confirm reductions:
#!/bin/bash set -euo pipefail echo "Remaining @ts-expect-error in experimental router:" rg -n "@ts-expect-error" packages/router/src/experimental/router.ts -n -C2 || trueAlso applies to: 736-858, 1082-1155, 1314-1331
packages/router/src/router.ts (2)
97-101: Fix incorrect JSDoc reference to experimental typeReplace EXPERIMENTAL_RouteRecordRaw with RouteRecordRaw to match the parameter type.
- * Add a new {@link EXPERIMENTAL_RouteRecordRaw | route record} as the child of an existing route. + * Add a new {@link RouteRecordRaw | route record} as the child of an existing route.
108-112: Fix incorrect JSDoc reference to experimental typeSame as above.
- * Add a new {@link EXPERIMENTAL_RouteRecordRaw | route record} to the router. + * Add a new {@link RouteRecordRaw | route record} to the router.
🧹 Nitpick comments (9)
packages/router/src/experimental/router.ts (8)
615-617: Harden scrollRestoration guard for non-browser/limited environmentsAccessing history.scrollRestoration without checking the property can throw in restricted contexts. Align with stable router’s defensive check.
Apply this diff:
- if (isBrowser && options.scrollBehavior) { - history.scrollRestoration = 'manual' - } + if (isBrowser && options.scrollBehavior && 'scrollRestoration' in history) { + history.scrollRestoration = 'manual' + }
694-705: Improve dev warning for unmatched locationsFor object locations, interpolating
${to}yields “[object Object]”. Use JSON.stringify for clarity.- if (!matchedRoute.matched.length) { - warn(`No match found for location with path "${to}"`) - } + if (!matchedRoute.matched.length) { + warn(`No match found for location ${JSON.stringify(to)}`) + }
707-714: Remove ts-expect-error and explicitly type matched for mergeMetaFieldsExplicitly cast matched to the experimental normalized record array to compute meta without suppressions.
- // matchedRoute is always a new object - // @ts-expect-error: FIXME: the `matched` property is different - return assign(matchedRoute, { + // matchedRoute is always a new object + return assign(matchedRoute, { redirectedFrom: undefined, href, - meta: mergeMetaFields(matchedRoute.matched), + meta: mergeMetaFields( + matchedRoute.matched as unknown as EXPERIMENTAL_RouteRecordNormalized[] + ), })
736-747: Return typed redirect intent and centralize resolve() at call sitehandleRedirectRecord currently calls resolve() and carries ts-expect-error. Returning the redirect “intent” with the same shape as resolve’s first arg lets callers resolve with proper currentLocation and removes suppressions.
- function handleRedirectRecord( + function handleRedirectRecord( to: RouteLocation, from: RouteLocationNormalizedLoaded - ): RouteLocationRaw | void { + ): _resolveArgs[0] | void { @@ - if (redirect) { - return resolve( - // @ts-expect-error: TODO: allow redirect to return the first argument of resolve or a tuple consisting of the arguments? - typeof redirect === 'function' ? redirect(to, from) : redirect, - from - ) - } + if (redirect) { + const target = + typeof redirect === 'function' ? redirect(to, from) : redirect + return (target as unknown) as _resolveArgs[0] + }And drop the suppression at the call site:
- // @ts-expect-error: FIXME: refactor location types - ...resolve(shouldRedirect, currentRoute.value), + ...resolve(shouldRedirect, currentRoute.value),Also applies to: 761-768
844-858: Resolve guard-provided redirects with typed target (remove suppression)Use the derived resolve input type to avoid ts-expect-error.
- // @ts-expect-error: FIXME: refactor location types - ...resolve(failure.to, currentRoute.value), + ...resolve(failure.to as _resolveArgs[0], currentRoute.value),
1096-1103: Remove suppression in history redirect flow; type the resolve target- // @ts-expect-error: FIXME: refactor location types - resolve(shouldRedirect), + resolve(shouldRedirect as _resolveArgs[0]),
1142-1146: Type the history-redirect target in setupListenersReliably handle NavigationRedirectError by typing the “to” before passing to resolve.
- // @ts-expect-error: to should be an absolute location - (error as NavigationRedirectError).to + (error as NavigationRedirectError).to as _resolveArgs[0]
29-29: Avoid circular deps: drop unused type import from stable routerThis file imports
type Routerfrom '../router' but doesn’t use it (only in a commented line), and router.ts imports types from this file. Remove this import to reduce the chance of circular dep hazards.-import type { Router } from '../router'packages/router/src/router.ts (1)
897-899: Decouple stable router from experimental type for onReady callbacksImporting _OnReadyCallback from experimental/router couples stable and experimental modules. Move the tuple alias to a neutral place (e.g., ./utils/callbacks.ts or ./types) and import it from there to avoid type cycles.
Example:
// packages/router/src/types/onReady.ts export type OnReadyCallback = [resolve: () => void, reject: (reason?: any) => void] // usage import type { OnReadyCallback } from './types/onReady' let readyHandlers = useCallbacks<OnReadyCallback>()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/router/src/experimental/router.ts(1 hunks)packages/router/src/router.ts(8 hunks)packages/router/src/typed-routes/route-records.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T15:22:32.526Z
Learnt from: posva
PR: vuejs/router#2415
File: packages/router/src/experimental/index.ts:43-44
Timestamp: 2025-08-11T15:22:32.526Z
Learning: In the Vue Router codebase, files within the src directory should not import from src/index to avoid circular dependencies. Deep imports like `../matcher/types` are intentional and necessary for maintaining proper dependency hierarchy.
Applied to files:
packages/router/src/router.ts
🧬 Code Graph Analysis (3)
packages/router/src/typed-routes/route-records.ts (2)
packages/router/src/typed-routes/route-location.ts (3)
RouteLocation(239-242)RouteLocationNormalizedLoaded(258-262)RouteLocationRaw(303-312)packages/router/src/index.ts (3)
RouteLocation(77-77)RouteLocationNormalizedLoaded(90-90)RouteLocationRaw(76-76)
packages/router/src/router.ts (2)
packages/router/src/experimental/router.ts (3)
EXPERIMENTAL_RouterOptions_Base(104-181)EXPERIMENTAL_Router_Base(411-571)_OnReadyCallback(93-93)packages/router/src/utils/callbacks.ts (1)
useCallbacks(4-24)
packages/router/src/experimental/router.ts (9)
packages/router/src/scrollBehavior.ts (7)
RouterScrollBehavior(35-46)saveScrollPosition(180-185)getScrollKey(173-176)computeScrollPosition(90-93)_ScrollPositionNormalized(26-30)getSavedScrollPosition(187-192)scrollToPosition(95-171)packages/router/src/experimental/route-resolver/resolver-fixed.ts (4)
EXPERIMENTAL_ResolverRecord_Base(27-53)EXPERIMENTAL_ResolverRecord_Matchable(71-75)EXPERIMENTAL_ResolverRecord_Group(58-66)EXPERIMENTAL_ResolverFixed(84-85)packages/router/src/errors.ts (10)
NavigationFailure(58-74)_ErrorListener(215-221)createRouterError(124-148)ErrorTypes(89-95)ErrorTypes(96-103)ErrorTypes(104-106)ErrorTypes(107-109)ErrorTypes(110-112)NavigationRedirectError(81-85)isNavigationFailure(184-193)packages/router/src/utils/callbacks.ts (1)
useCallbacks(4-24)packages/router/src/location.ts (1)
START_LOCATION_NORMALIZED(304-315)packages/router/src/utils/env.ts (1)
isBrowser(1-1)packages/router/src/experimental/route-resolver/resolver-abstract.ts (4)
ResolverLocationResolved(108-125)ResolverLocationAsPathRelative(170-183)ResolverLocationAsNamed(147-158)ResolverLocationAsRelative(212-225)packages/router/src/utils/index.ts (3)
assign(46-46)isArray(68-69)noop(62-62)packages/router/src/navigationGuards.ts (3)
extractChangingRecords(417-447)extractComponentsGuards(234-354)guardToPromiseFn(123-213)
🔇 Additional comments (3)
packages/router/src/typed-routes/route-records.ts (2)
4-4: LGTM: added import aligns with new redirect callback signatureImporting RouteLocationNormalizedLoaded is correct and consistent with its usage in the redirect function type.
14-17: Makefromoptional to preserve backward compatibility for separately-typed callbacksChanging the redirect function to require a second parameter can break code where a redirect function is pre-typed as
(to) => ...and later assigned toRouteRecordRedirectOptionunderstrictFunctionTypes. Makingfromoptional retains the new capability without risking subtle assignment breaks, while still allowing the router to always pass it at runtime.Apply this diff:
- | (( - to: RouteLocation, - from: RouteLocationNormalizedLoaded - ) => RouteLocationRaw) + | (( + to: RouteLocation, + from?: RouteLocationNormalizedLoaded + ) => RouteLocationRaw)[ suggest_optional_refactor ]
[ request_verification ]
To gauge impact, you can search for patterns that may be affected (single-parameter functions explicitly typed then assigned to RouteRecordRedirectOption) and inline redirect functions with a single param:#!/bin/bash # 1) Variables typed as RouteRecordRedirectOption and assigned a single-parameter function rg -nP -C2 '(?:const|let|var)\s+\w+\s*:\s*RouteRecordRedirectOption\s*=\s*\(\s*[^,)]*\)\s*=>' # 2) Inline redirect properties with a single-parameter function rg -nP -C2 'redirect\s*:\s*\(\s*[^,)]*\)\s*=>'packages/router/src/router.ts (1)
151-154: LGTM: safer scrollRestoration guardNice parity with the defensive check. This avoids exceptions in restricted environments.
| EXPERIMENTAL_RouterOptions_Base, | ||
| EXPERIMENTAL_Router_Base, | ||
| _OnReadyCallback, | ||
| } from './experimental/router' | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use type-only import from experimental router to avoid runtime circular deps
These are purely types; import them with import type to prevent bundlers/tsc from generating a runtime import, which would exacerbate the circular reference (this file <-> experimental/router.ts).
-import {
- EXPERIMENTAL_RouterOptions_Base,
- EXPERIMENTAL_Router_Base,
- _OnReadyCallback,
-} from './experimental/router'
+import type {
+ EXPERIMENTAL_RouterOptions_Base,
+ EXPERIMENTAL_Router_Base,
+ _OnReadyCallback,
+} from './experimental/router'Follow-up: consider moving _OnReadyCallback to a neutral module (e.g., ./utils/callbacks or ./types) to avoid any type coupling between stable and experimental surfaces.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| EXPERIMENTAL_RouterOptions_Base, | |
| EXPERIMENTAL_Router_Base, | |
| _OnReadyCallback, | |
| } from './experimental/router' | |
| import type { | |
| EXPERIMENTAL_RouterOptions_Base, | |
| EXPERIMENTAL_Router_Base, | |
| _OnReadyCallback, | |
| } from './experimental/router' |
🤖 Prompt for AI Agents
In packages/router/src/router.ts around lines 71 to 75, the imported identifiers
from ./experimental/router are only used as types, which can cause an
unnecessary runtime import and worsen circular dependencies; change the import
to a type-only import (use "import type" for EXPERIMENTAL_RouterOptions_Base,
EXPERIMENTAL_Router_Base, and _OnReadyCallback) so TypeScript erases the runtime
require, and as a follow-up consider moving _OnReadyCallback into a neutral
types module (e.g., ./types or ./utils/callbacks) and import it from there to
decouple stable and experimental surfaces.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/router/src/experimental/router.ts (4)
266-312: Typo from previous iteration is fixed: Normalized base is correctly namedEXPERIMENTAL_RouteRecordNormalized_Base and its extends are now correctly spelled. Good cleanup of a previously reported issue.
614-646: Resolve(): remove brittle @ts-expect-error and honor default current location
- Derive resolver parameter types directly from resolver.resolve.
- Default currentLocation to currentRoute.value when not provided (as per the method docs).
- Remove the two ts-expect-error suppressions.
Apply this diff:
type _resolveArgs = @@ - | [resolvedLocation: RouteLocationResolved, currentLocation?: undefined] + | [resolvedLocation: RouteLocationResolved, currentLocation?: undefined] + + // derive resolver argument types from the concrete resolver + type _ResolverTo = Parameters<typeof resolver.resolve>[0] + type _ResolverCur = Parameters<typeof resolver.resolve>[1] @@ function resolve( ...[to, currentLocation]: _resolveArgs ): RouteLocationResolved { - const matchedRoute = resolver.resolve( - // @ts-expect-error FIXME: incompatible types - to, - // FIXME: incompatible `matched` requires casting - currentLocation - ) + const effectiveCurrent = + (currentLocation ?? + // NOTE: resolver current location differs from Router's one; cast through unknown + (currentRoute.value as unknown)) as _ResolverCur + const matchedRoute = resolver.resolve( + to as _ResolverTo, + effectiveCurrent + )Also applies to: 647-655
808-822: Correct variable in NAVIGATION_GUARD_REDIRECT branchThis now correctly uses failure.to (not shouldRedirect). Good catch and fix over prior iteration.
1308-1314: Typing for globalProperties.$router: prefer module augmentation over suppressionAvoid shipping with @ts-expect-error. Add a module augmentation for the experimental router.
Apply this (in packages/router/src/experimental/index.ts):
// expose EXPERIMENTAL_Router on component instances when using the experimental build declare module 'vue' { export interface ComponentCustomProperties { $router: EXPERIMENTAL_Router // add $route here too if your experimental RouteLocation differs from the stable one } }Then this assignment compiles without suppression. Alternatively, cast once with a documented adapter type if augmentation isn’t desirable.
🧹 Nitpick comments (11)
packages/router/src/experimental/router.ts (11)
601-604: Harden scroll restoration guard to avoid edge casesAccess to history.scrollRestoration can fail in restricted contexts. Add a property existence check.
Apply this diff:
- if (isBrowser && options.scrollBehavior) { - history.scrollRestoration = 'manual' - } + if (isBrowser && options.scrollBehavior && 'scrollRestoration' in history) { + history.scrollRestoration = 'manual' + }
666-668: Nit: make “no match” warning robust for non-string inputsto can be an object; stringify for clarity and consistency with the warning above.
Apply this diff:
- if (!matchedRoute.matched.length) { - warn(`No match found for location with path "${to}"`) - } + if (!matchedRoute.matched.length) { + warn(`No match found for location ${JSON.stringify(to)}`) + }
671-677: Resolve(): replace suppression with an explicit, local type adaptationThe only mismatch is the matched property’s element type. Adapt it locally and return the exact RouteLocationResolved type.
Apply this diff:
- // matchedRoute is always a new object - // @ts-expect-error: FIXME: the `matched` property is different - return assign(matchedRoute, { - redirectedFrom: undefined, - href, - meta: mergeMetaFields(matchedRoute.matched), - }) + // matchedRoute is always a new object; adapt matched to experimental records + return assign( + matchedRoute as unknown as Omit<RouteLocationResolved, 'matched'> & { + matched: EXPERIMENTAL_RouteRecordNormalized[] + }, + { + redirectedFrom: undefined, + href, + meta: mergeMetaFields(matchedRoute.matched), + } + ) as RouteLocationResolved
704-711: handleRedirectRecord(): type the resolver call and drop suppressionUse the derived resolver parameter types to remove the ts-expect-error.
Apply this diff:
- if (redirect) { - return resolver.resolve( - // @ts-expect-error: TODO: allow redirect to return the first argument of resolve or a tuple consisting of the arguments? - typeof redirect === 'function' ? redirect(to, from) : redirect, - from - ) - } + if (redirect) { + const target = + (typeof redirect === 'function' ? redirect(to, from) : redirect) as _ResolverTo + return resolver.resolve(target, from as unknown as _ResolverCur) + }
725-742: Redirect branch: type mismatch persists; consider an adapter helpershouldRedirect is typed as RouteLocationRaw, which doesn’t exactly match your resolve() overloads. Given this is a hot path, prefer a small adapter that translates a RouteLocationRaw into the accepted relative/named/path union for your resolver-backed resolve() to avoid broad casts or suppressions.
Example (outside this file) create a toResolverLocation(raw: RouteLocationRaw): string | ResolverLocationAsRelative | ResolverLocationAsPathRelative | ResolverLocationAsNamed and use it here to replace the ts-expect-error.
I can draft a small, covered adapter with unit tests if you want to take this approach.
786-799: Infinite redirect detection: ensure same-route check uses the effective current locationWith the resolve() fix to default currentLocation, resolve(failure.to) will now behave as expected. If you prefer being explicit, pass currentRoute.value to remove any ambiguity.
Apply this optional diff:
- isSameRouteLocation( + isSameRouteLocation( stringifyQuery, - // @ts-expect-error: FIXME: failure.to should not contain relative locations - resolve(failure.to), + resolve(failure.to, currentRoute.value), toLocation ) &&
1061-1068: History listener redirect: same type mismatch as aboveThe ts-expect-error here comes from resolve(shouldRedirect). Address via the same adapter suggested for pushWithRedirect, or leverage a narrowed cast once that adapter exists.
Happy to include this call site in the adapter change and update related tests.
1361-1364: Devtools integration typingaddDevtools expects stable types; either widen its signature to accept the experimental router and resolver, or cast with a documented adapter. Don’t leave the suppression long term.
128-176: Docs consistency: clarify stringifyQuery default and parseQuery storySince parseQuery is intentionally not exposed here (commented in destructuring), consider adding a short note in the experimental docs clarifying that custom query parsing/serialization should be implemented via the resolver (e.g., a root record param that parses the whole query). Helps users migrating from createRouter options.
I can draft the doc blurb to keep behavior aligned with the new resolver model.
384-391: Resolver type parameter narrownessEXPERIMENTAL_RouterOptions.resolver is hard-coded to EXPERIMENTAL_ResolverFixed<EXPERIMENTAL_RouteRecordNormalized_Matchable>. If you foresee multiple resolver flavors (dynamic, pattern-based, etc.), consider a generic on EXPERIMENTAL_RouterOptions (and carry it through EXPERIMENTAL_Router_Base) to avoid future refactors.
I can sketch a minimal generic threading that preserves current defaults while allowing alternative resolvers.
1289-1295: Clean up@ts-expect-errorsuppressions after EXPERIMENTAL_Router types are alignedThere are still several
@ts-expect-errorcomments inpackages/router/src/experimental/router.tstied to the public API surface. Once theresolve()refactor lands and you’ve updatedEXPERIMENTAL_Router_Baseto the correct call signatures, please remove all of these suppressions and fix the underlying types:• API methods export (around lines 1284–1292):
– resolve, push, replace, go
•app.config.globalProperties.$routerassignment (lines 1308–1310)
•app.provide(routerKey, router)&app.provide(routeLocationKey, …)(lines 1340–1342)
• Devtools integration calladdDevtools(app, router, resolver)(lines 1362–1363)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/router/src/experimental/router.ts(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/router/src/experimental/route-resolver/matchers/param-parsers/numbers.ts (1)
12-13: Integer parser set() must validate (NaN, Infinity, non-integers)
set()currently serializes any number, includingNaN,Infinity, and non-integers. This breaks the get/set symmetry and can construct invalid routes.Apply:
- set: (value: number) => String(value), + set: (value: number) => { + if (!Number.isFinite(value) || !Number.isInteger(value)) throw miss() + return String(value) + },packages/router/src/experimental/route-resolver/resolver-fixed.ts (1)
221-223: Named/object resolution should prioritize explicit to.hash over record defaultsUsers expect an explicit
to.hashto win. Current order usesrecord.hash?.build(params)first, shadowingto.hashwhen non-nullish (even empty string). Swap precedence:- const hash = - record.hash?.build(params) ?? to.hash ?? currentLocation?.hash ?? '' + const hash = + to.hash ?? + (to.name == null ? currentLocation?.hash : record.hash?.build(params)) ?? + record.hash?.build(params) ?? + ''Also consider whether
''fromrecord.hash?.buildshould be treated as "absent" (use??vs||)—right now an empty string prevents fallback.
🧹 Nitpick comments (7)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern-query.ts (3)
1-1: Avoid hard dependency on Vue for simple default resolutionUsing
toValuepullsvueinto this module just to resolve a default value. This increases coupling and bundle surface. Consider a local helper to handle(() => T) | T(or a MaybeRefOrGetter if you want to support refs), and remove the import.Apply:
-import { toValue } from 'vue'And add a local helper (example):
// inside this file (top-level) function resolveDefault<T>(def?: (() => T) | T): T | undefined { if (typeof def === 'function') return (def as () => T)() return def }Then replace
toValue(this.defaultValue)withresolveDefault(this.defaultValue)below.
97-101: Guard against missing parser.set to avoid runtime errors
this.parser.set!is asserted butParamParser.setis optional. If a consumer provides a read-only parser (onlyget), this will throw at runtime. Provide a safe fallback or a dev-only assertion.Option A (dev assert):
return { - // FIXME: default setter - [this.queryKey]: this.parser.set!(paramValue), + [this.queryKey]: + (__DEV__ && !this.parser.set + ? (() => { + throw new Error('Param parser is missing a set() implementation') + })() + : this.parser.set + )!(paramValue), }Option B (fallback passthrough if you accept identity):
return { - [this.queryKey]: this.parser.set!(paramValue), + [this.queryKey]: this.parser.set + ? this.parser.set(paramValue) + : (paramValue as unknown as string | string[] | null), }Please confirm which behavior you prefer across the experimental API surface.
49-71: Array parsing: partial successes leak through silently when defaultValue is presentWith
format: 'array' | 'both', invalid entries are skipped if a default is provided, potentially returning a partially parsed array without signaling errors. This may surprise users and breaks round-trip guarantees when some inputs are invalid. Consider either:
- Rejecting the whole array on any invalid entry when default is present, or
- Exposing a configurable strategy: 'skip-invalid' (current) | 'fail-fast' | 'use-default' (replace whole array by default).
If you keep current behavior, document it in the JSDoc.
packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts (1)
21-29: Default parser semantics: clarify null handling for arrays
PARAM_PARSER_DEFAULTS.setmaps arrays tovalue.map(v => (v == null ? null : String(v))), which allows null elements. Ensure this aligns withLocationQueryconstraints and upstream serializers. If not, coalesce nullish to empty strings as the rest of the query machinery often does.packages/router/src/experimental/route-resolver/matchers/param-parsers/numbers.ts (1)
5-11: Edge case: empty string should be rejected, but current check depends on truthiness
if (value && Number.isInteger(num))relies on truthiness ofvalue. This works for '0' (truthy) but might be fragile if later extended. Consider explicitly checkingvalue != null && value !== ''for clarity.- if (value && Number.isInteger(num)) { + if (value != null && value !== '' && Number.isInteger(num)) {packages/router/src/experimental/route-resolver/resolver-fixed.ts (2)
224-232: Order of query merges: let explicit to.query override record-built defaultsThe current merge applies record-built query objects after user-provided
to.query, which can override explicit inputs. Typically, explicit inputs should take precedence over defaults. Suggest reordering:- const query = Object.assign( - { - ...currentLocation?.query, - ...normalizeQuery(to.query), - }, - ...matched.flatMap(record => - record.query?.map(query => query.build(params)) - ) - ) + const query = Object.assign( + {}, + // record/query defaults first + ...matched.flatMap(record => record.query?.map(q => q.build(params))), + // then current location + currentLocation?.query, + // finally explicit overrides + normalizeQuery(to.query) + )If the current behavior is intentional, consider adding a brief doc comment explaining why record-built values win over explicit input.
114-117: Duplicate record names silently overwrite in recordMapWhen two records share a name, the later one overwrites the former with no signal. In dev, warn on duplicates to prevent hard-to-debug mismatches.
for (const record of records) { - recordMap.set(record.name, record) + if (__DEV__ && recordMap.has(record.name)) { + warn(`Duplicate resolver record name "${String(record.name)}" detected. The last one wins.`) + } + recordMap.set(record.name, record) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
packages/experiments-playground/src/router/index.ts(1 hunks)packages/router/src/experimental/index.ts(1 hunks)packages/router/src/experimental/route-resolver/matchers/matcher-pattern-query.ts(1 hunks)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.test-d.ts(1 hunks)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts(1 hunks)packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts(1 hunks)packages/router/src/experimental/route-resolver/matchers/param-parsers/numbers.ts(1 hunks)packages/router/src/experimental/route-resolver/matchers/param-parsers/types.ts(1 hunks)packages/router/src/experimental/route-resolver/matchers/test-utils.ts(1 hunks)packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts(1 hunks)packages/router/src/experimental/route-resolver/resolver-fixed.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts
- packages/router/src/experimental/route-resolver/matchers/matcher-pattern.test-d.ts
- packages/router/src/experimental/route-resolver/matchers/param-parsers/types.ts
- packages/router/src/experimental/route-resolver/matchers/test-utils.ts
- packages/router/src/experimental/index.ts
- packages/experiments-playground/src/router/index.ts
- packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-14T13:58:50.446Z
Learnt from: posva
PR: vuejs/router#2415
File: packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts:0-0
Timestamp: 2025-08-14T13:58:50.446Z
Learning: The `encodeParam` function in vue-router handles nullish values (null/undefined) by returning an empty string, rather than stringifying them to "null" or "undefined". This means missing parameters in route building result in empty path segments rather than literal "undefined" in URLs.
Applied to files:
packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts
📚 Learning: 2025-08-14T13:58:50.446Z
Learnt from: posva
PR: vuejs/router#2415
File: packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts:0-0
Timestamp: 2025-08-14T13:58:50.446Z
Learning: The `encodeParam` function in vue-router has the signature `(text: string | number | null | undefined): string` and specifically handles nullish values by returning an empty string via `text == null ? '' : encodePath(text).replace(SLASH_RE, '%2F')`. This means missing or null parameters in route building result in empty path segments rather than literal "null"/"undefined" strings in URLs.
Applied to files:
packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts
🧬 Code Graph Analysis (4)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern-query.ts (2)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (4)
MatcherParamsFormatted(255-255)MatcherPattern(19-41)MatcherQueryParams(267-267)EmptyParams(260-260)packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts (1)
ParamParser(41-41)
packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts (1)
packages/router/src/experimental/route-resolver/matchers/param-parsers/types.ts (1)
ParamParser(9-15)
packages/router/src/experimental/route-resolver/resolver-fixed.ts (6)
packages/router/src/experimental/route-resolver/resolver-abstract.ts (8)
RecordName(10-10)EXPERIMENTAL_Resolver_Base(19-91)ResolverLocationResolved(108-125)ResolverLocationAsPathAbsolute(196-199)ResolverLocationAsPathRelative(170-183)ResolverLocationAsNamed(147-158)ResolverLocationAsRelative(212-225)NO_MATCH_LOCATION(131-135)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (4)
MatcherPatternPath(47-50)MatcherPatternHash(246-248)MatcherQueryParams(267-267)MatcherParamsFormatted(255-255)packages/router/src/experimental/route-resolver/matchers/matcher-pattern-query.ts (1)
MatcherPatternQuery(15-17)packages/router/src/location.ts (4)
LocationNormalized(13-18)NEW_stringifyURL(120-128)parseURL(44-97)resolveRelativePath(241-287)packages/router/src/warning.ts (1)
warn(2-9)packages/router/src/query.ts (1)
normalizeQuery(131-148)
packages/router/src/experimental/route-resolver/matchers/param-parsers/numbers.ts (1)
packages/router/src/experimental/route-resolver/matchers/param-parsers/types.ts (1)
ParamParser(9-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
packages/router/src/experimental/route-resolver/matchers/matcher-pattern-query.ts
Show resolved
Hide resolved
| return parser | ||
| } | ||
|
|
||
| export const PATH_PARAM_SINGLE_DEFAULT: ParamParser<string, string> = {} |
There was a problem hiding this comment.
Empty parser object will crash at call sites
PATH_PARAM_SINGLE_DEFAULT is {}, but downstream code asserts parser.get!/parser.set!. Supplying this constant will cause runtime errors. Either remove it or provide meaningful no-op implementations.
One safe option:
-export const PATH_PARAM_SINGLE_DEFAULT: ParamParser<string, string> = {}
+export const PATH_PARAM_SINGLE_DEFAULT: ParamParser<string, string> = {
+ get: (value: string) => value,
+ set: (value: string) => value,
+}If you do not intend to use it, consider deleting it to avoid accidental use.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const PATH_PARAM_SINGLE_DEFAULT: ParamParser<string, string> = {} | |
| export const PATH_PARAM_SINGLE_DEFAULT: ParamParser<string, string> = { | |
| get: (value: string) => value, | |
| set: (value: string) => value, | |
| } |
🤖 Prompt for AI Agents
In
packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts
around line 16, PATH_PARAM_SINGLE_DEFAULT is currently an empty object which
causes runtime crashes where callers use parser.get!/parser.set!; replace or
remove it — preferably replace with a safe no-op ParamParser implementation that
provides concrete get and set functions (typed to ParamParser<string,string>)
that simply return the input value (identity) and, if required by the interface,
provide any default metadata (e.g., name/description) so downstream code can
call parser.get and parser.set without null checks; alternatively, if this
default is not used anywhere, delete the exported constant to prevent accidental
use.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/router/src/experimental/route-resolver/matchers/param-parsers/booleans.ts (1)
25-30: Minor type consistency: allow nulls in repeatable optional input.To match the non-optional repeatable variant (which tolerates nulls in the input array) and the matcher’s behavior around null elements, make the TRaw accept (string | null)[] | null. This improves symmetry and avoids friction if used outside MatcherPatternQuery.
Apply:
export const PARAM_BOOLEAN_REPEATABLE_OPTIONAL = { - get: (value: string[] | null) => + get: (value: (string | null)[] | null) => value == null ? null : PARAM_BOOLEAN_REPEATABLE.get(value), set: (value: boolean[] | null) => value != null ? PARAM_BOOLEAN_REPEATABLE.set(value) : null, -} satisfies ParamParser<boolean[] | null, string[] | null> +} satisfies ParamParser<boolean[] | null, (string | null)[] | null>packages/router/src/experimental/route-resolver/matchers/matcher-pattern-query.spec.ts (3)
169-170: Fix typo in test title (“und efined” → “undefined”).Minor spelling nit in the test description.
- it('strips off und efined values', () => { + it('strips off undefined values', () => {
310-322: Add a negative case for boolean parser.There is no test for invalid boolean strings. Add one to ensure miss() propagates when no default is provided.
it('can use PARAM_PARSER_BOOL for booleans', () => { const matcher = new MatcherPatternQueryParam( 'enabled', 'e', 'value', PARAM_PARSER_BOOL ) expect(matcher.match({ e: 'true' })).toEqual({ enabled: true }) expect(matcher.match({ e: 'false' })).toEqual({ enabled: false }) expect(matcher.build({ enabled: false })).toEqual({ e: 'false' }) expect(matcher.build({ enabled: true })).toEqual({ e: 'true' }) }) + it('throws on invalid boolean without default', () => { + const matcher = new MatcherPatternQueryParam( + 'enabled', + 'e', + 'value', + PARAM_PARSER_BOOL + ) + expect(() => matcher.match({ e: 'not-a-bool' })).toThrow(MatchMiss) + })
183-205: Consider adding an array-format test for booleans.To mirror number-array coverage, add a case for PARAM_PARSER_BOOL with format: 'array' to ensure build/match work with ['true','false'] and single values coerced to arrays.
Example addition (for guidance):
it('handles boolean arrays with PARAM_PARSER_BOOL', () => { const matcher = new MatcherPatternQueryParam('flags', 'f', 'array', PARAM_PARSER_BOOL) expect(matcher.match({ f: ['true', 'false'] })).toEqual({ flags: [true, false] }) expect(matcher.match({ f: 'true' })).toEqual({ flags: [true] }) expect(matcher.build({ flags: [true, false] })).toEqual({ f: ['true', 'false'] }) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/router/src/experimental/index.ts(1 hunks)packages/router/src/experimental/route-resolver/matchers/matcher-pattern-query.spec.ts(1 hunks)packages/router/src/experimental/route-resolver/matchers/matcher-pattern-query.ts(1 hunks)packages/router/src/experimental/route-resolver/matchers/param-parsers/booleans.ts(1 hunks)packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts
- packages/router/src/experimental/route-resolver/matchers/matcher-pattern-query.ts
- packages/router/src/experimental/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern-query.spec.ts (3)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern-query.ts (1)
MatcherPatternQueryParam(19-110)packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts (2)
PARAM_PARSER_DEFAULTS(21-29)PARAM_PARSER_BOOL(45-45)packages/router/src/experimental/route-resolver/matchers/param-parsers/booleans.ts (1)
PARAM_PARSER_BOOL(37-50)
packages/router/src/experimental/route-resolver/matchers/param-parsers/booleans.ts (2)
packages/router/src/experimental/index.ts (3)
miss(45-45)ParamParser(41-41)PARAM_PARSER_BOOL(40-40)packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts (2)
ParamParser(42-42)PARAM_PARSER_BOOL(45-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
packages/router/src/experimental/route-resolver/matchers/param-parsers/booleans.ts (3)
4-11: Boolean single parser: strict and correct.Strictly accepting only 'true' and 'false' and delegating errors to miss() is appropriate. set() produces canonical string forms.
20-23: Repeatable parser is fine; relies on upstream null filtering.Mapping through PARAM_BOOLEAN_SINGLE.get is correct given matcher-pattern-query filters out nulls before invoking the parser for arrays.
37-50: Dispatcher looks good and matches the ParamParser contract.The array/non-array branching is correct, null is preserved, and set() mirrors get(). Works across 'value', 'array', and 'both' formats.
packages/router/src/experimental/route-resolver/matchers/matcher-pattern-query.spec.ts (3)
145-157: Good coverage for build() with value format.Assertions verify correct key mapping and null handling, aligning with PARAM_PARSER_DEFAULTS behavior.
277-287: Numeric parser integration: LGTM.Covers both match and build paths for PARAM_PARSER_INT. Clear and precise.
289-297: Error propagation check is correct.Asserting MatchMiss on invalid input without a default ensures failures aren’t silently swallowed.
New version of the internal matcher (renamed as resolver). With more responsibilities and allowing it to be overridden:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests
Chores