-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Custom Route Resolvers #2415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Custom Route Resolvers #2415
Conversation
✅ Deploy Preview for vue-router canceled.
|
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2415 +/- ##
==========================================
- Coverage 94.90% 94.84% -0.06%
==========================================
Files 34 34
Lines 3002 3028 +26
Branches 846 851 +5
==========================================
+ Hits 2849 2872 +23
- Misses 150 153 +3
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
[skip ci]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/router/src/experimental/route-resolver/resolver-dynamic.ts (3)
1-8
: Resolved: relative imports are consistent with codebase conventionsSwitching to relative imports here aligns with the repository’s dependency hierarchy and avoids circular deps per the learnings. Looks good.
Also applies to: 20-21
113-116
: Resolved: use of warn() instead of console.warnUsing the imported warn() maintains consistency with the rest of the codebase.
269-276
: removeMatcher() leaves stale entries and parent referencesCurrently only matcherMap is updated. Stale entries remain in matchers; parent.children keeps references; aliases aren’t cleaned. This causes future resolutions to still hit removed routes.
Apply this diff to fully remove a matcher, its aliases, and detach it from its parent:
function removeMatcher(matcher: TMatcherRecord) { matcherMap.delete(matcher.name) - for (const child of matcher.children) { + // remove children first + for (const child of [...matcher.children]) { removeMatcher(child) } - // TODO: delete from matchers - // TODO: delete children and aliases + // detach from parent.children + if (matcher.parent) { + const idx = matcher.parent.children.indexOf(matcher) + if (idx > -1) matcher.parent.children.splice(idx, 1) + } + // remove this matcher and any aliases pointing to it + for (let i = matchers.length - 1; i >= 0; i--) { + const m = matchers[i] + if (m === matcher || m.aliasOf === matcher) { + matchers.splice(i, 1) + } + } + // clear own children to avoid stale references + matcher.children = [] }
🧹 Nitpick comments (7)
packages/router/vitest.config.ts (3)
28-30
: Remove redundant exclusion; keep only the glob.
'src/experimental/**/*.ts'
already matches'src/experimental/index.ts'
. Dropping the specific entry avoids duplication and keeps the list lean.- 'src/experimental/index.ts',
29-29
: Use an actionable TODO with ownership and intent.Replace the generic FIXME with a TODO that states when to remove the exclusion and who owns the follow-up.
- // FIXME: add more tests + // TODO(router): add tests for experimental resolver and remove this exclusion before stabilizing
30-30
: Consider tracking experimental coverage separately instead of blanket exclusion.Excluding the entire experimental tree hides coverage signal for the new resolver while it evolves. A separate Vitest config (or project) dedicated to experimental code can keep main coverage stable without losing visibility.
Example: add a dedicated config to run in CI as a non-blocking job.
// packages/router/vitest.experimental.config.ts import { defineConfig } from 'vitest/config' import Vue from '@vitejs/plugin-vue' export default defineConfig({ plugins: [Vue()], test: { coverage: { include: ['src/experimental/**/*.ts'], exclude: [ 'src/**/*.d.ts', 'src/**/*.test-d.ts', 'src/**/*.spec.ts', ], // optional: separate report dir to avoid clobbering main // reportsDirectory: './coverage-experimental', }, typecheck: { enabled: true, checker: 'vue-tsc', }, }, })Then wire a script like
vitest --config vitest.experimental.config.ts --run
in CI for visibility without affecting the main coverage gate.packages/router/src/experimental/route-resolver/resolver-dynamic.ts (4)
283-286
: Return a copy from getRecords() to prevent external mutationExpose a defensive copy to avoid accidental external mutations of the internal ordering.
function getRecords() { - return matchers + return matchers.slice() }
55-57
: Typo: “Prevoisly” → “Previously”- * Remove all matcher records. Prevoisly named `clearRoutes()`. + * Remove all matcher records. Previously named `clearRoutes()`.
134-137
: Typo: “catched” → “caught”- // either one of them must be defined and is catched by the dev only warn above + // either one of them must be defined and is caught by the dev-only warn above
375-379
: Remove stray debug/TODO comment noiseThese comments appear to be leftover scratch notes and add noise.
-} // pathEncoded`/users/${1}` -// TODO: -// pathEncoded`/users/${null}/end` -// const a: RouteRecordRaw = {} as any +}
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/router/src/experimental/route-resolver/resolver-dynamic.ts
(1 hunks)packages/router/vitest.config.ts
(1 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/experimental/route-resolver/resolver-dynamic.ts
🔇 Additional comments (1)
packages/router/src/experimental/route-resolver/resolver-dynamic.ts (1)
147-156
: Named/relative object resolution — honor explicit hash and let user query override matcher-built defaultsShort: prefer an explicit to.hash over matcher-built or current hash, and build query so matcher defaults are overridden by currentLocation and finally by to.query (user wins). I inspected resolver-dynamic.ts and found the same pattern in resolver-static.ts — please run the router test suite to confirm no tests depend on the current precedence before landing.
Files to change:
- packages/router/src/experimental/route-resolver/resolver-dynamic.ts (named/relative branch)
- packages/router/src/experimental/route-resolver/resolver-static.ts (named/relative branch) — apply the same change for parity
Suggested diffs:
packages/router/src/experimental/route-resolver/resolver-dynamic.ts
- const path = matcher.path.build(params) - const hash = matcher.hash?.build(params) ?? '' - const matched = buildMatched(matcher) - const query = Object.assign( - { - ...currentLocation?.query, - ...normalizeQuery(to.query), - }, - ...matched.map(matcher => matcher.query?.build(params)) - ) + const path = matcher.path.build(params) + const matched = buildMatched(matcher) + const query = Object.assign( + {}, + ...matched.map(m => m.query?.build(params)), + currentLocation?.query, + normalizeQuery(to.query) + ) + const hash = + (to as ResolverLocationAsNamed | ResolverLocationAsRelative).hash ?? + matcher.hash?.build(params) ?? + currentLocation?.hash ?? + ''packages/router/src/experimental/route-resolver/resolver-static.ts
- const path = record.path.build(params) - const hash = record.hash?.build(params) ?? '' - const matched = buildMatched(record) - const query = Object.assign( - { - ...currentLocation?.query, - ...normalizeQuery(to.query), - }, - ...matched.flatMap(record => - record.query?.map(query => query.build(params)) - ) - ) + const path = record.path.build(params) + const matched = buildMatched(record) + const query = Object.assign( + {}, + ...matched.flatMap(r => r.query?.map(q => q.build(params))), + currentLocation?.query, + normalizeQuery(to.query) + ) + const hash = + (to as ResolverLocationAsNamed | ResolverLocationAsRelative).hash ?? + record.hash?.build(params) ?? + currentLocation?.hash ?? + ''Action for reviewer / maintainer:
- Run the router test suite (and any relevant resolver/matcher tests). If any tests rely on matcher-built query values overriding user-specified ones, keep the previous order; otherwise, apply these changes to both files for consistent, user-first semantics.
packages/router/src/experimental/route-resolver/resolver-dynamic.ts
Outdated
Show resolved
Hide resolved
packages/router/src/experimental/route-resolver/resolver-dynamic.ts
Outdated
Show resolved
Hide resolved
packages/router/src/experimental/route-resolver/resolver-dynamic.ts
Outdated
Show resolved
Hide resolved
kept because it will be fixed and shiped at some point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 10
♻️ Duplicate comments (5)
packages/router/src/experimental/router.ts (5)
281-281
: Fix typo in interface name:EXPERIMENTAL_RouteRecordNoramlized_Base
→EXPERIMENTAL_RouteRecordNormalized_Base
The interface name is misspelled. This typo should be fixed to avoid confusion and maintain consistency.
Apply this diff to fix the typo:
-export interface EXPERIMENTAL_RouteRecordNoramlized_Base { +export interface EXPERIMENTAL_RouteRecordNormalized_Base {
311-311
: Update extends clauses to use the corrected interface nameAfter fixing the typo in the base interface name, update the extends clauses in these two interfaces.
Apply this diff:
export interface EXPERIMENTAL_RouteRecordNormalized_Group - extends EXPERIMENTAL_RouteRecordNoramlized_Base, + extends EXPERIMENTAL_RouteRecordNormalized_Base, EXPERIMENTAL_RouteRecord_Group { meta: RouteMeta parent: EXPERIMENTAL_RouteRecordNormalized | null } // TODO: is it worth to have 2 types for the undefined values? export interface EXPERIMENTAL_RouteRecordNormalized_Matchable - extends EXPERIMENTAL_RouteRecordNoramlized_Base, + extends EXPERIMENTAL_RouteRecordNormalized_Base, EXPERIMENTAL_RouteRecord_Matchable { meta: RouteMetaAlso applies to: 319-319
675-703
: Address TypeScript compatibility issues in resolve functionMultiple type compatibility issues are suppressed with
@ts-expect-error
comments. These should be properly resolved with type mappings or adapters.Consider creating proper type mappings between the experimental resolver types and existing router types to eliminate these suppressions. The type mismatches on lines 676-677, 679, and 697-698 indicate a fundamental incompatibility between the resolver's expected types and what's being passed.
870-884
: Critical: Using wrong variable in redirect logicIn the
NAVIGATION_GUARD_REDIRECT
branch, the code usesshouldRedirect
(from line 787) instead offailure.to
when resolving the redirect location. This will cause incorrect redirects.Apply this diff to fix the redirect logic:
return pushWithRedirect( { - // @ts-expect-error: FIXME: refactor location types - ...resolve(shouldRedirect, currentRoute.value), + // target the guard-provided redirect + ...resolve(failure.to, currentRoute.value), state: typeof failure.to === 'object' ? assign({}, data, failure.to.state) : data, force, },
1367-1373
: Resolve global properties typing issueThe
@ts-expect-error
on line 1367 indicates a type mismatch betweenEXPERIMENTAL_Router
and the expectedRouter
type for$router
. This should be properly resolved with module augmentation.Add proper module augmentation in
packages/router/src/experimental/index.ts
:declare module 'vue' { export interface ComponentCustomProperties { $router: EXPERIMENTAL_Router $route: RouteLocationNormalizedLoaded } }Alternatively, use a type-safe cast:
type RouterAdapter = { $router: EXPERIMENTAL_Router } & Record<string, unknown> ;(app.config.globalProperties as unknown as RouterAdapter).$router = router
🧹 Nitpick comments (14)
packages/router/src/experimental/route-resolver/matcher-resolve.spec.ts (5)
34-46
: Consider using existing EXPERIMENTAL_RouteRecordRaw type instead of redefining it.This interface extends
NEW_MatcherRecordRaw
with the same properties that are likely already defined elsewhere in the experimental router system. Since this is a test file, consider importing the actual type if it exists to maintain consistency.
68-112
: Add explicit return type annotation tocompileRouteRecord
function.The function returns
NEW_MatcherRecordRaw
but lacks an explicit return type annotation, which would improve type safety and code documentation.function compileRouteRecord( record: RouteRecordRaw, parentRecord?: RouteRecordRaw -): NEW_MatcherRecordRaw { +): NEW_MatcherRecordRaw {
74-76
: Improve error message for routes without absolute paths.The error message could be more specific about which route is causing the issue to aid debugging.
if (!parentRecord && !record.path.startsWith('/')) { - throw new Error(`Record without parent must have an absolute path`) + throw new Error(`Record "${record.name || record.path}" without parent must have an absolute path`) }
86-86
: Remove commented-out console.log statements.Debug logging statements should be removed from test code before merging.
- // console.log({ record, parser }) return { group: !isMatchable(record), name: record.name, score: parser.score, path: { match(value) { const params = parser.parse(value) - // console.log('🌟', parser.re, value, params) if (params) { return params }Also applies to: 96-96
429-459
: Several TODO tests should be reviewed for implementation priority.Multiple tests are marked with
.todo
which means they're not being executed:
- Line 429: "keep optional params from parent record"
- Line 462: "discards non existent params" (has implementation but marked as todo)
- Line 509: "warns if a path isn not absolute"
Consider prioritizing which of these tests should be implemented for the experimental resolver to be feature-complete.
Would you like help implementing any of these pending test cases?
Also applies to: 509-515
packages/router/src/experimental/route-resolver/old/resolver-dynamic.test-d.ts (1)
43-43
: Fix typo in test description.There's a typo in the test description: "resolved" should be "resolves".
- it('resolved named locations', () => { + it('resolves named locations', () => {packages/router/src/experimental/route-resolver/old/resolver-dynamic.spec.ts (2)
19-27
: Consider extracting the PAGE_QUERY_PATTERN_MATCHER to test utilities.This matcher pattern could be useful for other tests. Consider moving it to the test-utils file alongside other test matchers.
103-103
: Complete the TODO test suite for absolute locations as strings.The
describe.todo
block indicates incomplete test coverage for absolute string locations. This is an important test case that should be implemented.Would you like me to help implement the complete test suite for absolute locations as strings? The current tests inside seem functional but the describe block is marked as todo.
packages/router/src/experimental/route-resolver/resolver-abstract.ts (2)
151-164
: Consider improving error handling in decode function.The decode function catches errors but only logs in dev mode. Consider returning the original text wrapped in a result type or throwing a more descriptive error for production to help with debugging encoding issues.
export function decode( text: string | number | null | undefined ): string | null { if (text == null) return null try { return decodeURIComponent('' + text) } catch (err) { - __DEV__ && warn(`Error decoding "${text}". Using original value`) + if (__DEV__) { + warn(`Error decoding "${text}". Using original value. Error: ${err}`) + } } return '' + text }
189-192
: Remove or properly type the commented-out encodeQueryValue overload.The
@ts-expect-error
comment and the overload definition suggest incomplete implementation. Either implement it properly or remove it.-// @ts-expect-error: overload are not correctly identified -const encodeQueryValue: FnStableNull = - // for ts - value => (value == null ? null : _encodeQueryValue(value)) +const encodeQueryValue: FnStableNull = ((value: string | number | null | undefined) => + value == null ? null : _encodeQueryValue(value)) as FnStableNullpackages/router/src/experimental/route-resolver/old/resolver-dynamic.ts (2)
375-378
: Remove commented-out code at the end of the file.These comments appear to be leftover development notes and should be removed.
-} // pathEncoded`/users/${1}` -// TODO: -// pathEncoded`/users/${null}/end` -// const a: RouteRecordRaw = {} as any +}
112-132
: Improve error message for unnamed relative location warning.The error message could be more actionable by suggesting what the user should do.
warn( - `Cannot resolve an unnamed relative location without a current location. This will throw in production.`, + `Cannot resolve an unnamed relative location without a current location. Please provide either a 'name' field or a currentLocation parameter. This will throw in production.`, to )packages/router/src/experimental/router.ts (2)
305-307
: Consider removing Options API dependencyThe comment indicates that the
instances
property is only needed for Options API navigation guards. Consider documenting a migration path or timeline for removing this dependency.Since this is marked as "FIXME", consider:
- Adding a deprecation notice if Options API support will be removed
- Providing a migration guide for users relying on Options API guards
- Making this property optional if it's truly only for Options API
919-921
: Consider extracting app context logicThe
runWithContext
function retrieves the first installed app, which might not be the intended app in multi-app scenarios.Consider:
- Document that this uses the first installed app's context
- Consider passing the specific app context when available
- Add a warning in dev mode when multiple apps are installed
function runWithContext<T>(fn: () => T): T { const app: App | undefined = installedApps.values().next().value + if (__DEV__ && installedApps.size > 1) { + warn('Multiple apps detected. Using context from the first installed app.') + } return app?.runWithContext ? app.runWithContext(fn) : fn() }
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (9)
packages/router/src/experimental/index.ts
(1 hunks)packages/router/src/experimental/route-resolver/matcher-resolve.spec.ts
(1 hunks)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts
(1 hunks)packages/router/src/experimental/route-resolver/matchers/test-utils.ts
(1 hunks)packages/router/src/experimental/route-resolver/old/resolver-dynamic.spec.ts
(1 hunks)packages/router/src/experimental/route-resolver/old/resolver-dynamic.test-d.ts
(1 hunks)packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts
(1 hunks)packages/router/src/experimental/route-resolver/resolver-abstract.ts
(1 hunks)packages/router/src/experimental/router.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts
- packages/router/src/experimental/route-resolver/matchers/test-utils.ts
- packages/router/src/experimental/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/router/src/experimental/route-resolver/old/resolver-dynamic.test-d.ts (2)
packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts (2)
NEW_MatcherRecordRaw
(438-470)NEW_RouterResolver
(36-58)packages/router/src/experimental/route-resolver/resolver-abstract.ts (1)
ResolverLocationResolved
(109-114)
packages/router/src/experimental/route-resolver/old/resolver-dynamic.spec.ts (3)
packages/router/src/experimental/route-resolver/matchers/test-utils.ts (7)
EMPTY_PATH_ROUTE
(67-73)USER_ID_ROUTE
(83-89)ANY_PATH_PATTERN_MATCHER
(10-19)ANY_HASH_PATTERN_MATCHER
(61-65)USER_ID_PATH_PATTERN_MATCHER
(31-48)EMPTY_PATH_PATTERN_MATCHER
(21-29)ANY_PATH_ROUTE
(75-81)packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts (1)
createCompiledMatcher
(59-300)packages/router/src/experimental/route-resolver/resolver-abstract.ts (2)
pathEncoded
(213-226)NO_MATCH_LOCATION
(203-207)
packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts (5)
packages/router/src/experimental/route-resolver/resolver-abstract.ts (9)
EXPERIMENTAL_Resolver_Base
(23-92)RecordName
(14-14)ResolverLocationResolved
(109-114)ResolverLocationAsPathAbsolute
(254-257)ResolverLocationAsPathRelative
(239-252)ResolverLocationAsNamed
(227-238)ResolverLocationAsRelative
(258-271)NO_MATCH_LOCATION
(203-207)MatcherQueryParams
(124-124)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)packages/router/src/matcher/pathParserRanker.ts (1)
comparePathParserScore
(336-363)
packages/router/src/experimental/route-resolver/resolver-abstract.ts (4)
packages/router/src/location.ts (1)
LocationNormalized
(13-18)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (1)
MatcherParamsFormatted
(451-451)packages/router/src/warning.ts (1)
warn
(2-9)packages/router/src/encoding.ts (2)
encodeQueryValue
(88-101)encodeParam
(131-133)
packages/router/src/experimental/router.ts (12)
packages/router/src/experimental/index.ts (5)
EXPERIMENTAL_RouterOptions_Base
(6-6)EXPERIMENTAL_RouteRecord_Base
(9-9)EXPERIMENTAL_RouteRecordRaw
(8-8)EXPERIMENTAL_RouterOptions
(7-7)EXPERIMENTAL_Router
(4-4)packages/router/src/matcher/pathParserRanker.ts (1)
PathParserOptions
(86-89)packages/router/src/experimental/route-resolver/resolver-static.ts (4)
EXPERIMENTAL_ResolverRecord_Base
(30-58)EXPERIMENTAL_ResolverRecord_Matchable
(73-77)EXPERIMENTAL_ResolverRecord_Group
(63-71)EXPERIMENTAL_ResolverStatic
(86-87)packages/router/src/types/index.ts (3)
RouteMeta
(260-260)RawRouteComponent
(186-186)Lazy
(15-15)packages/router/src/errors.ts (8)
NavigationFailure
(58-74)createRouterError
(124-148)ErrorTypes
(89-95)ErrorTypes
(96-103)ErrorTypes
(104-106)ErrorTypes
(107-109)ErrorTypes
(110-112)isNavigationFailure
(184-193)packages/router/src/utils/callbacks.ts (1)
useCallbacks
(4-24)packages/router/src/location.ts (2)
START_LOCATION_NORMALIZED
(291-302)parseURL
(43-96)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 (1)
ResolverLocationResolved
(109-114)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)
packages/router/src/experimental/route-resolver/matcher-resolve.spec.ts (11)
packages/router/src/experimental/index.ts (4)
EXPERIMENTAL_RouteRecordRaw
(8-8)EXPERIMENTAL_RouterOptions
(7-7)MatcherPatternPath
(32-32)MatcherPatternPathStatic
(23-23)packages/router/src/experimental/router.ts (2)
EXPERIMENTAL_RouteRecordRaw
(273-275)EXPERIMENTAL_RouterOptions
(379-387)packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts (2)
NEW_MatcherRecordRaw
(438-470)NEW_MatcherRecord
(475-475)packages/router/src/matcher/pathParserRanker.ts (3)
tokensToParser
(128-293)PATH_PARSER_OPTIONS_DEFAULTS
(375-379)PathParams
(4-4)packages/router/src/matcher/pathTokenizer.ts (1)
tokenizePath
(46-200)packages/router/src/utils/index.ts (1)
mergeOptions
(71-81)packages/router/src/experimental/route-resolver/matchers/errors.ts (1)
miss
(15-15)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (2)
MatcherPatternPath
(47-50)MatcherPatternPathStatic
(63-81)packages/router/__tests__/vitest-mock-warn.ts (1)
mockWarn
(5-114)packages/router/src/experimental/route-resolver/resolver-abstract.ts (5)
ResolverLocationResolved
(109-114)MatcherLocationRaw
(97-103)ResolverLocationAsNamed
(227-238)ResolverLocationAsPathAbsolute
(254-257)NO_MATCH_LOCATION
(203-207)packages/router/src/location.ts (1)
NEW_stringifyURL
(107-115)
⏰ 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/experimental/route-resolver/matcher-resolve.spec.ts (1)
53-59
: Remove redundant type check inisMatchable
function.The redirect check on line 57 is unnecessary since redirect is not part of the RouteRecordRaw type being checked. This could be misleading about what makes a record matchable.
function isMatchable(record: RouteRecordRaw): boolean { return !!( record.name || - (record.components && Object.keys(record.components).length) || - record.redirect + (record.components && Object.keys(record.components).length) ) }Likely an incorrect or invalid review comment.
packages/router/src/experimental/route-resolver/old/resolver-dynamic.test-d.ts (1)
1-81
: LGTM! Comprehensive type-level tests for the resolver.The type tests thoroughly cover all resolution scenarios including absolute paths, relative paths, named locations, and proper error cases with
@ts-expect-error
directives. Good coverage of the type system constraints.packages/router/src/experimental/route-resolver/resolver-abstract.ts (1)
1-272
: LGTM! Well-structured resolver abstraction with comprehensive type definitions.The file provides a solid foundation for the experimental route resolver system with:
- Clear type definitions for different location formats
- Proper overloads for the resolve method
- Utility functions for encoding/decoding
- Good TypeScript practices with proper type constraints
packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts (1)
1-475
: LGTM! Well-implemented dynamic resolver with good architectural patterns.The implementation provides a robust dynamic route resolution system with:
- Efficient binary search for insertion
- Proper parent-child relationship management
- Comprehensive resolution logic for different location types
- Good separation of concerns
packages/router/src/experimental/router.ts (3)
93-97
: Well-documented internal typeGood use of JSDoc to clearly mark this as an internal type.
577-579
: Clear experimental API documentationExcellent documentation clearly marking this as experimental and explaining its purpose as a resolver-based alternative to the traditional routes array approach.
1438-1447
: Clean implementation of meta field mergingThe
mergeMetaFields
function provides a simple and effective way to merge meta fields from matched records using object spread.
packages/router/src/experimental/route-resolver/matcher-resolve.spec.ts
Outdated
Show resolved
Hide resolved
packages/router/src/experimental/route-resolver/matcher-resolve.spec.ts
Outdated
Show resolved
Hide resolved
expect.objectContaining({ | ||
path: '/parent/o/t', | ||
name: 'nestednested', | ||
matched: [ | ||
expect.objectContaining({ | ||
path: '/parent', | ||
aliasOf: undefined, | ||
}), | ||
expect.objectContaining({ | ||
path: '/parent/o', | ||
aliasOf: expect.objectContaining({ path: '/parent/one' }), | ||
}), | ||
expect.objectContaining({ | ||
path: '/parent/o/t', | ||
aliasOf: expect.objectContaining({ path: '/parent/one/two' }), | ||
}), | ||
], | ||
}) | ||
) | ||
}) | ||
|
||
it('resolves when all are aliases', () => { | ||
assertRecordMatch( | ||
record, | ||
{ path: '/p/o/t' }, | ||
expect.objectContaining({ | ||
path: '/p/o/t', | ||
name: 'nestednested', | ||
matched: [ | ||
expect.objectContaining({ | ||
path: '/p', | ||
aliasOf: expect.objectContaining({ path: '/parent' }), | ||
}), | ||
expect.objectContaining({ | ||
path: '/p/o', | ||
aliasOf: expect.objectContaining({ path: '/parent/one' }), | ||
}), | ||
expect.objectContaining({ | ||
path: '/p/o/t', | ||
aliasOf: expect.objectContaining({ path: '/parent/one/two' }), | ||
}), | ||
], | ||
}) | ||
) | ||
}) | ||
|
||
it('resolves when first and last are aliases', () => { | ||
assertRecordMatch( | ||
record, | ||
{ path: '/p/one/t' }, | ||
expect.objectContaining({ | ||
path: '/p/one/t', | ||
name: 'nestednested', | ||
matched: [ | ||
expect.objectContaining({ | ||
path: '/p', | ||
aliasOf: expect.objectContaining({ path: '/parent' }), | ||
}), | ||
expect.objectContaining({ | ||
path: '/p/one', | ||
aliasOf: expect.objectContaining({ path: '/parent/one' }), | ||
}), | ||
expect.objectContaining({ | ||
path: '/p/one/t', | ||
aliasOf: expect.objectContaining({ path: '/parent/one/two' }), | ||
}), | ||
], | ||
}) | ||
) | ||
}) | ||
}) | ||
|
||
it('resolves the original path of the named children of a route with an alias', () => { | ||
const children = [{ path: 'one', component, name: 'nested' }] | ||
assertRecordMatch( | ||
{ | ||
path: '/parent', | ||
alias: '/p', | ||
component, | ||
children, | ||
}, | ||
{ name: 'nested', params: {} }, | ||
{ | ||
path: '/parent/one', | ||
name: 'nested', | ||
params: {}, | ||
matched: [ | ||
// TODO: | ||
// { | ||
// path: '/parent', | ||
// children, | ||
// components, | ||
// aliasOf: undefined, | ||
// }, | ||
// { path: '/parent/one', name: 'nested', components }, | ||
], | ||
} | ||
) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enable and complete the skipped alias tests.
The entire alias test suite is marked as describe.skip
, which means these tests are not running. If aliases are not yet implemented, consider adding a TODO comment explaining when they will be implemented. If they are implemented but tests are failing, they should be fixed.
Would you like me to help investigate why these alias tests are skipped and potentially help fix or implement the missing functionality?
🤖 Prompt for AI Agents
In packages/router/src/experimental/route-resolver/matcher-resolve.spec.ts
around lines 705 to 1199 the entire alias test suite is skipped (describe.skip)
and many expected alias assertions are commented out; remove the .skip to enable
the suite, replace the commented TODO expectation blocks with the appropriate
expect or objectContaining assertions for aliasOf/path/name as shown in nearby
tests, and either fix the route-resolver implementation to populate aliasOf
correctly (so assertions pass) or, if aliases are not implemented, add a clear
TODO comment at the top of the suite explaining why the tests remain skipped and
when they will be enabled.
it('removes static path', () => { | ||
const matcher = createCompiledMatcher() | ||
matcher.addMatcher(EMPTY_PATH_ROUTE) | ||
matcher.removeMatcher(EMPTY_PATH_ROUTE) | ||
// Add assertions to verify the route was removed | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add assertions to verify route removal.
The remove tests are missing assertions to verify that the routes were actually removed from the matcher.
it('removes static path', () => {
const matcher = createCompiledMatcher()
matcher.addMatcher(EMPTY_PATH_ROUTE)
matcher.removeMatcher(EMPTY_PATH_ROUTE)
- // Add assertions to verify the route was removed
+ expect(matcher.getRecords()).toHaveLength(0)
+ expect(matcher.getRecord(EMPTY_PATH_ROUTE.name)).toBeUndefined()
})
it('removes dynamic path', () => {
const matcher = createCompiledMatcher()
matcher.addMatcher(USER_ID_ROUTE)
matcher.removeMatcher(USER_ID_ROUTE)
- // Add assertions to verify the route was removed
+ expect(matcher.getRecords()).toHaveLength(0)
+ expect(matcher.getRecord(USER_ID_ROUTE.name)).toBeUndefined()
})
Also applies to: 94-99
🤖 Prompt for AI Agents
In packages/router/src/experimental/route-resolver/old/resolver-dynamic.spec.ts
around lines 87-92 (and similarly for lines 94-99), the tests call
matcher.removeMatcher(...) but do not assert the route was actually removed;
update each test to assert removal by checking the matcher no longer matches
that route (e.g., call the matcher lookup/match method for the removed path and
assert it returns null/undefined/false or that the route count/registry length
decreased), and add any necessary setup checks to confirm the route existed
before removal so the assertions validate the removal.
packages/router/src/experimental/route-resolver/old/resolver-dynamic.spec.ts
Outdated
Show resolved
Hide resolved
packages/router/src/experimental/route-resolver/old/resolver-dynamic.spec.ts
Outdated
Show resolved
Hide resolved
const name = record.name ?? (__DEV__ ? Symbol('unnamed-route') : Symbol()) | ||
// FIXME: proper normalization of the record | ||
// @ts-expect-error: we are not properly normalizing the record yet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Address the FIXME comment about proper record normalization.
The comment indicates that the record is not being properly normalized. This could lead to type safety issues and runtime errors.
🏁 Script executed:
#!/bin/bash
# Check what fields might be missing in the normalization
ast-grep --pattern 'interface NEW_MatcherRecordRaw {
$$$
}' packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts
ast-grep --pattern 'interface NEW_MatcherRecord {
$$$
}' packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts
Length of output: 3809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts"
echo "===== lines 1-320 ====="
sed -n '1,320p' "$file" || true
echo
echo "===== lines 321-640 ====="
sed -n '321,640p' "$file" || true
echo
echo "===== rg matches ====="
rg -n "FIXME|proper normalization|@ts-expect-error|normalize|NEW_MatcherRecord|NEW_MatcherRecordRaw|record.name" "$file" || true
echo
echo "===== exported declarations ====="
rg -n "export (interface|type|function|const|let|var|class)" "$file" || true
Length of output: 17736
Fix required: properly normalize NEW_MatcherRecordRaw into NEW_MatcherRecord (remove the @ts-expect-error)
The code currently casts raw records to normalized records without guaranteeing required fields or compiled MatcherPattern objects exist — this is unsafe (type and runtime risks). Implement a proper normalization step instead of silencing TypeScript.
Files/locations to fix
- packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts — addMatcher (lines ~237-241): replace the current ad-hoc spread + @ts-expect-error with a normalization helper.
Current snippet:Replace with a call to a function that returns a fully-populated NEW_MatcherRecord (no ts-expect-error).const name = record.name ?? (__DEV__ ? Symbol('unnamed-route') : Symbol()) // FIXME: proper normalization of the record // @ts-expect-error: we are not properly normalizing the record yet const normalizedRecord: TMatcherRecord = { ...record, name, parent, children: [], }
- packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts — type definitions (lines ~394, 427, 438–475): NEW_MatcherRecordRaw, NEW_MatcherRecord, NEW_MatcherDynamicRecord and EXPERIMENTAL_ResolverRecord_Base show required properties (e.g., non-optional name, children array, score, and MatcherPattern objects used with .build/.match). Ensure normalization fills these.
Minimum normalization responsibilities
- Ensure name is non-null (assign default Symbol).
- Ensure children is a mutable array and parent is set.
- Compile/validate path, query and hash into MatcherPattern objects that expose build() and match() (or otherwise adapt callers).
- Compute/assign score (NEW_MatcherDynamicRecord requires it).
- Preserve/handle aliasOf/group semantics and any other router-specific invariants.
- Remove the @ts-expect-error and keep the code type-safe.
Suggested quick approach
- Add normalizeMatcherRecord(raw, parent?): TMatcherRecord that performs the above (compile patterns, defaults, compute score), then use:
const normalizedRecord = normalizeMatcherRecord(record, parent)
Reason: resolve() and matching logic call matcher.path.build/match and matcher.query?.build/match; those rely on fully-normalized matcher objects — leaving this unchecked will cause runtime errors and unsafe casts.
function removeMatcher(matcher: TMatcherRecord) { | ||
matcherMap.delete(matcher.name) | ||
for (const child of matcher.children) { | ||
removeMatcher(child) | ||
} | ||
// TODO: delete from matchers | ||
// TODO: delete children and aliases | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complete the removeMatcher implementation.
The function has TODO comments indicating incomplete implementation for removing from the matchers array and handling aliases.
function removeMatcher(matcher: TMatcherRecord) {
matcherMap.delete(matcher.name)
for (const child of matcher.children) {
removeMatcher(child)
}
- // TODO: delete from matchers
- // TODO: delete children and aliases
+ const index = matchers.indexOf(matcher)
+ if (index > -1) {
+ matchers.splice(index, 1)
+ }
+ // Remove any aliases pointing to this matcher
+ for (let i = matchers.length - 1; i >= 0; i--) {
+ if (isAliasRecord(matchers[i]) && matchers[i].aliasOf === matcher) {
+ matchers.splice(i, 1)
+ }
+ }
}
📝 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.
function removeMatcher(matcher: TMatcherRecord) { | |
matcherMap.delete(matcher.name) | |
for (const child of matcher.children) { | |
removeMatcher(child) | |
} | |
// TODO: delete from matchers | |
// TODO: delete children and aliases | |
} | |
function removeMatcher(matcher: TMatcherRecord) { | |
matcherMap.delete(matcher.name) | |
for (const child of matcher.children) { | |
removeMatcher(child) | |
} | |
const index = matchers.indexOf(matcher) | |
if (index > -1) { | |
matchers.splice(index, 1) | |
} | |
// Remove any aliases pointing to this matcher | |
for (let i = matchers.length - 1; i >= 0; i--) { | |
if (isAliasRecord(matchers[i]) && matchers[i].aliasOf === matcher) { | |
matchers.splice(i, 1) | |
} | |
} | |
} |
🤖 Prompt for AI Agents
In packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts
around lines 269 to 276, the removeMatcher implementation needs to also remove
the matcher from the shared matchers array and drop any alias entries; update
the function to (1) find and splice out the matcher from the top-level matchers
array (or remove any references to it in that collection), (2) iterate
matcher.aliases (or matcher.alias) and delete each alias from the alias map
(aliasMap / aliasToMatcher), (3) recursively call removeMatcher for children
(already present) and ensure children are also removed from the matchers array,
and (4) clear matcher.children and matcher.aliases to avoid retained references.
Ensure you use the same collection/map variable names used elsewhere in the file
when removing entries and handle missing entries defensively.
if (isBrowser && options.scrollBehavior) { | ||
history.scrollRestoration = 'manual' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Guard against missing history.scrollRestoration in non-browser environments
While isBrowser
check exists, accessing history.scrollRestoration
could still throw in some edge cases (e.g., restricted iframe contexts).
Apply this defensive check:
// leave the scrollRestoration if no scrollBehavior is provided
-if (isBrowser && options.scrollBehavior) {
+if (isBrowser && options.scrollBehavior && 'scrollRestoration' in history) {
history.scrollRestoration = 'manual'
}
📝 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.
if (isBrowser && options.scrollBehavior) { | |
history.scrollRestoration = 'manual' | |
} | |
if (isBrowser && options.scrollBehavior && 'scrollRestoration' in history) { | |
history.scrollRestoration = 'manual' | |
} |
🤖 Prompt for AI Agents
In packages/router/src/experimental/router.ts around lines 596 to 598, the code
sets history.scrollRestoration = 'manual' guarded only by isBrowser; add a
defensive check that history is defined and has a writable scrollRestoration
property before assigning to it. Update the condition to ensure typeof history
!== 'undefined' and 'scrollRestoration' in history and that assignment is safe
(e.g., try a typeof check or property existence check) and only then set
history.scrollRestoration = 'manual'; keep the isBrowser guard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.git-crypt/.gitattributes (1)
3-4
: Optional hardening for attributesIf you keep this, consider also disabling EOL normalization in this directory to prevent any platform-specific transforms on key material:
-* !filter !diff +* !filter !diff -textAdditionally, ensure the directory is excluded from published npm artifacts (via files field or .npmignore) to avoid shipping crypto metadata in packages.
📜 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)
.git-crypt/.gitattributes
(1 hunks).gitattributes
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitattributes
⏰ 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)
.git-crypt/.gitattributes (1)
1-4
: Keep — git-crypt metadata is intentional; do not remove .git-crypt/.gitattributesVerification shows git-crypt is configured in this repo, so the file in this PR is not accidental:
- .gitattributes (repo root): contains "**/CLAUDE.md filter=git-crypt diff=git-crypt" (found by ripgrep).
- .git-crypt/.gitattributes (present; matches PR):
# Do not edit this file. To specify the files to encrypt, create your own # .gitattributes file in the directory where your files are. * !filter !diff *.gpg binary
- .git-crypt/keys/default/0/12C6A8C197D70636AC2F3C0B5CFCEAB47F7E9844.gpg — binary key file present.
Conclusion: the original review comment is incorrect — this metadata is part of the repository's git-crypt setup and should remain.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/router/src/experimental/route-resolver/old/matcher-resolve.spec.ts (8)
61-66
: joinPaths produces 'undefined/' and an extra trailing slash for empty child paths.If a is undefined, joinPaths returns strings like 'undefined/foo'. If b is an empty string (e.g., nested routes with ''), it returns '/parent/' instead of '/parent'. Safer handling avoids surprising slashes.
Apply this diff:
-export function joinPaths(a: string | undefined, b: string) { - if (a?.endsWith('/')) { - return a + b - } - return a + '/' + b -} +export function joinPaths(a: string | undefined, b: string) { + if (!a) return b + if (!b) return a + return a.endsWith('/') ? a + b : a + '/' + b +}
74-85
: Avoid mutating input records in compileRouteRecord.Mutating record.path has side effects that can surprise callers and complicate future reuse. Compute an absolute path locally and pass a normalized parent down to children.
Apply this diff:
- const path = record.path.startsWith('/') - ? record.path - : joinPaths(parentRecord?.path, record.path) - record.path = path - const parser = tokensToParser( - tokenizePath(record.path), + const absolutePath = record.path.startsWith('/') + ? record.path + : joinPaths(parentRecord?.path, record.path) + const parser = tokensToParser( + tokenizePath(absolutePath), mergeOptions(PATH_PARSER_OPTIONS_DEFAULTS, record) ) @@ - children: record.children?.map(childRecord => - compileRouteRecord(childRecord, record) - ), + children: record.children?.map(childRecord => + compileRouteRecord(childRecord, { ...record, path: absolutePath }) + ),Also applies to: 105-108
53-59
: Treat single-component records as matchable.Records that only define component (singular) should also be considered matchable. Current guard only checks components and misses component.
Apply this diff:
-function isMatchable(record: RouteRecordRaw): boolean { - return !!( - record.name || - (record.components && Object.keys(record.components).length) || - record.redirect - ) -} +function isMatchable(record: RouteRecordRaw): boolean { + return !!( + record.name || + record.component || + (record.components && Object.keys(record.components).length) || + record.redirect + ) +}
139-143
: Strengthen the type guard for experimental records.Using typeof record.path !== 'string' is brittle. Guard against the actual MatcherPatternPath shape (presence of match and build).
Apply this diff:
- return typeof record.path !== 'string' + return ( + record != null && + typeof record.path === 'object' && + record.path !== null && + 'match' in record.path && + 'build' in record.path + )
196-203
: Only hidematched
from assertions when it’s not part of expectations.Right now, matched is always set to a non-enumerable empty array, which prevents asserting it even when provided in expectedLocation. Keep the current behavior by default, but allow assertions when the test provides matched.
Apply this diff:
- Object.defineProperty(resolved, 'matched', { - writable: true, - configurable: true, - enumerable: false, - // FIXME: build it - value: [], - }) + if (!('matched' in expectedLocation)) { + Object.defineProperty(resolved, 'matched', { + writable: true, + configurable: true, + enumerable: false, + // FIXME: build it + value: [], + }) + }
81-85
: Limit parser options passed to tokensToParser for clarity.Passing the entire record relies on mergeOptions ignoring unrelated keys. Being explicit improves readability and type-safety.
You can refactor this portion:
- const parser = tokensToParser( - tokenizePath(absolutePath), - mergeOptions(PATH_PARSER_OPTIONS_DEFAULTS, record) - ) + const parser = tokensToParser( + tokenizePath(absolutePath), + mergeOptions(PATH_PARSER_OPTIONS_DEFAULTS, { + strict: (record as any).strict, + end: (record as any).end, + sensitive: (record as any).sensitive, + }) + )If RouteRecordRaw already includes these fields, drop the casts and rely on types.
501-507
: Typo in test description.Fix “isn not” → “is not”.
Apply this diff:
- it.todo('warns if a path isn not absolute', () => { + it.todo('warns if a path is not absolute', () => {
371-497
: Add query/hash resolution tests to align with the PR objective.Given the resolver now supports matching based on query (and custom parsing/serialization), consider adding cases that:
- Resolve by name/path with query filters that match and that don’t match.
- Verify query serialization consistency in fullPath.
- Include hash handling.
I can draft these tests to match the new resolver behavior 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 (3)
packages/router/src/experimental/route-resolver/old/matcher-resolve.spec.ts
(1 hunks)packages/router/src/experimental/route-resolver/resolver-static.spec.ts
(1 hunks)packages/router/src/experimental/route-resolver/resolver-static.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/router/src/experimental/route-resolver/resolver-static.ts
- packages/router/src/experimental/route-resolver/resolver-static.spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router/src/experimental/route-resolver/old/matcher-resolve.spec.ts (7)
packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts (3)
NEW_MatcherRecordRaw
(438-470)createCompiledMatcher
(59-300)NEW_MatcherRecord
(475-475)packages/router/src/matcher/pathParserRanker.ts (3)
tokensToParser
(128-293)PATH_PARSER_OPTIONS_DEFAULTS
(375-379)PathParams
(4-4)packages/router/src/matcher/pathTokenizer.ts (1)
tokenizePath
(46-200)packages/router/src/utils/index.ts (1)
mergeOptions
(71-81)packages/router/src/experimental/route-resolver/matchers/errors.ts (1)
miss
(15-15)packages/router/src/experimental/route-resolver/resolver-abstract.ts (4)
ResolverLocationResolved
(109-114)ResolverLocationAsNamed
(227-238)ResolverLocationAsPathAbsolute
(254-257)NO_MATCH_LOCATION
(203-207)packages/router/src/location.ts (1)
NEW_stringifyURL
(107-115)
⏰ 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/old/matcher-resolve.spec.ts (2)
111-126
: Strong coverage and clear scaffolding for resolve scenarios (LGTM).Good breadth across path, name, and relative resolution, including repeatable and optional params, strict mode, and children. The helpers (compileRouteRecord, assertRecordMatch) make the intent easy to follow.
Also applies to: 229-369
1-1
: No.spec.ts
files are published
All test files remain undersrc
and aren’t referenced by the build. Thefiles
field inpackages/router/package.json
only includes:
dist/**/*.{js,cjs,mjs}
dist/**/*.d.{ts,mts}
- specific top‐level artifacts (e.g.
index.js
, type files, Vetur assets, README)The build (via
tsdown
) only emits the specified entry points (src/index.ts
andsrc/experimental/index.ts
), so.spec.ts
files never enterdist
or the published package. No action required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/router/package.json (1)
8-8
: Ship CJS typings too (.d.cts) or confirm resolution strategyYou’ve switched to ESM declarations via .d.mts at the package level. Ensure CommonJS consumers also get correct typings (typically by shipping a .d.cts alongside). Otherwise, CJS projects and TS NodeNext users may see resolution quirks.
🧹 Nitpick comments (3)
packages/router/package.json (3)
30-30
: Add types for the experimental exportSubpath "./experimental" points to an ESM file but lacks a types mapping. Add an explicit types path to improve TS resolution and editor UX.
Apply this diff:
- "./experimental": "./dist/experimental/index.mjs", + "./experimental": { + "types": "./dist/experimental/index.d.mts", + "import": "./dist/experimental/index.mjs" + },If the declaration file isn’t emitted yet, we can adjust the build to output dist/experimental/index.d.mts.
86-87
: Include .d.cts in the published files if you generate CJS declarationsIf you decide to ship CommonJS declaration files, make sure they’re published too.
Apply this diff:
- "dist/**/*.d.{ts,mts}", + "dist/**/*.d.{ts,mts,cts}",
95-98
: Make build:dts idempotent to avoid duplicate appendsCurrent tail >> dist/vue-router.d.mts will append on every run, creating duplicate global extensions in repeated builds/tests.
Apply this diff to delegate to a small script that checks before appending:
- "build:dts": "tail -n +10 src/globalExtensions.ts >> dist/vue-router.d.mts", + "build:dts": "node scripts/append-global-extensions.mjs",And add scripts/append-global-extensions.mjs:
// scripts/append-global-extensions.mjs import { readFileSync, writeFileSync, existsSync } from 'node:fs'; const out = 'dist/vue-router.d.mts'; const marker = '// @vue-router:globalExtensions-appended'; const src = 'src/globalExtensions.ts'; // Ensure base declaration exists (built by `pnpm run build`) if (!existsSync(out)) { throw new Error(`Missing ${out}. Run "pnpm run build" before build:dts.`); } const content = readFileSync(out, 'utf8'); if (content.includes(marker)) { // already appended, nothing to do process.exit(0); } // drop first 9 lines to keep parity with previous tail -n +10 const srcContent = readFileSync(src, 'utf8').split('\n').slice(9).join('\n'); writeFileSync( out, `${content}\n\n${marker}\n${srcContent}\n`, 'utf8' ); console.log('Appended globalExtensions to vue-router.d.mts');
📜 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 ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
packages/experiments-playground/README.md
(1 hunks)packages/experiments-playground/package.json
(1 hunks)packages/experiments-playground/src/router/index.ts
(1 hunks)packages/router/package.json
(5 hunks)packages/router/src/experimental/index.ts
(1 hunks)packages/router/src/experimental/route-resolver/matchers/errors.ts
(1 hunks)packages/router/src/experimental/route-resolver/matchers/test-utils.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/experiments-playground/README.md
- packages/router/src/experimental/route-resolver/matchers/test-utils.ts
- packages/router/src/experimental/route-resolver/matchers/errors.ts
- packages/experiments-playground/package.json
- packages/experiments-playground/src/router/index.ts
- packages/router/src/experimental/index.ts
🧰 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/package.json
⏰ 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 (5)
packages/router/package.json (5)
11-17
: LGTM: Node import conditions to root-level vue-router.node.mjsThe conditional ESM entrypoints pointing at ./vue-router.node.mjs look consistent. This also aligns with including vue-router.node.mjs in "files".
35-37
: LGTM: Consistent node import mapping for ./auto-routesMatches the main entry’s approach and keeps behavior consistent across subpaths.
52-54
: LGTM: Consistent node import mapping for ./autoSame as above; consistent and clear.
83-83
: LGTM: Ensure the new root ESM entry is publishedIncluding vue-router.node.mjs in "files" makes the new export work for consumers.
123-154
: Toolchain bumps look fine; confirm CI Node/TS matrixUpgrades to Vite 7, Vue 3.5, and the @typescript/native-preview suggest a modern toolchain. Ensure CI uses Node/TS versions compatible with these (Node ≥18 for Vite 7; TS preview pinned already).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (3)
440-454
: Remove or implement the commented-outbuild()
The method is still commented. Either drop it or implement it (with proper
set
+encodeParam
usage and required-param handling) to avoid dead code in the experimental surface.I can help implement it consistent with the
ParamParser
contract and the currentopts
semantics if you want to go that route.
159-165
: Avoid emitting literal "null"/"undefined" in URLs when using defaultset
PATH_PARAM_DEFAULT_SET
converts nullish values to the strings"null"
/"undefined"
. This conflicts withencodeParam
’s nullish handling (which intentionally yields an empty segment). Returnnull
for nullish input so downstream encoding works as intended.Apply this diff:
-const PATH_PARAM_DEFAULT_SET = (value: string | string[] | null | undefined) => - value && Array.isArray(value) ? value.map(String) : String(value) +const PATH_PARAM_DEFAULT_SET = (value: string | string[] | null | undefined) => + value == null + ? null + : Array.isArray(value) + ? value.map(String) + : String(value)
105-121
: Preserve original casing in star prefix; keep a separate lowercase cache for matching
MatcherPatternPathStar
lowercases and stores the prefix inthis.path
, sobuild()
emits a lower-cased prefix (e.g.,/MyTeam
→/myteam
). Mirror the static matcher pattern: keep the original prefix for building and a lowercased cache for matching.Apply this diff:
- private path: string - constructor(path: string = '') { - this.path = path.toLowerCase() - } + private path: string + private pathi: string + constructor(path: string = '') { + this.path = path + this.pathi = path.toLowerCase() + } @@ - match(path: string): { pathMatch: string } { - if (!path.toLowerCase().startsWith(this.path)) { + match(path: string): { pathMatch: string } { + if (!path.toLowerCase().startsWith(this.pathi)) {
🧹 Nitpick comments (3)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (3)
323-330
: Fix dev warning to report accurate counts (capture groups vs defined params)The current message uses
match.length
(includes the full match) and printsi
but checks object key lengths. Compute group count asmatch.length - 1
and compare againstthis.paramsKeys.length
.Apply this diff:
- 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 matchedCount = match.length - 1 + const definedCount = this.paramsKeys.length + if (matchedCount !== definedCount) { + warn( + `Regexp matched ${matchedCount} params, but ${definedCount} params are defined. Found when matching "${path}" against ${String(this.re)}` + ) + } + }
89-101
: Typo: “folllowed” → “followed”Minor doc fix.
Apply this diff:
- * Allows matching a static path folllowed by anything. + * Allows matching a static path followed by anything.
405-413
: Typo in docs: “patch” → “path”Minor doc fix.
Apply this diff:
- * @throws if the patch doesn't match + * @throws if the path doesn't match
📜 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/matcher-pattern.spec.ts
(1 hunks)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router/src/experimental/route-resolver/matchers/matcher-pattern.spec.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 (1)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (5)
packages/router/src/experimental/index.ts (13)
MatcherPattern
(30-30)MatcherParamsFormatted
(34-34)MatcherPatternPath
(32-32)MatcherPatternPathStatic
(23-23)EmptyParams
(35-35)miss
(39-39)ParamParser
(36-36)PARAM_PARSER_INT
(27-27)MatcherPatternPathCustomParams
(25-25)MatcherPatternPathDynamic
(22-22)MatcherPatternQuery
(33-33)MatcherQueryParams
(18-18)MatcherPatternHash
(31-31)packages/router/src/experimental/route-resolver/resolver-abstract.ts (2)
decode
(154-164)MatcherQueryParams
(124-124)packages/router/src/utils/index.ts (1)
identityFn
(15-15)packages/router/src/warning.ts (1)
warn
(2-9)packages/router/src/encoding.ts (1)
encodeParam
(131-133)
⏰ 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/matcher-pattern.ts (2)
66-85
: Nice: static path keeps original casing, while matching remains case-insensitiveStoring
pathi
for comparisons and returning the originalpath
inbuild()
addresses the earlier casing issue for static paths.
223-237
: LGTM: integer regex accepts zero and avoids rejecting valid integers
/^-?\d+$/
plusNumber.isFinite()
yields the expected integer validation behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/router/src/experimental/index.ts (1)
45-45
: Fix typo in comment.“realying” → “relying”.
-// this should create type errors if someone is realying on children +// this should create type errors if someone is relying on childrenpackages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (1)
73-86
: Preserves original casing while matching case-insensitively — good fixStoring the original
path
and a lowercased copy (pathi
) ensuresmatch()
is case-insensitive whilebuild()
returns the original casing. This addresses prior casing concerns without mutating consumer input.
🧹 Nitpick comments (13)
packages/router/src/experimental/route-resolver/matchers/param-parsers/types.ts (1)
20-22
: Clarify optional/null handling in ParamParser_Generic.
ParamParser_Generic
covers single vs repeatable inputs but excludesnull
. If the intent is “non-optional path params only”, consider clarifying the JSDoc; otherwise, include the optional case.Apply one of:
- * Generic type for a param parser that can handle both single and repeatable params. + * Generic type for a param parser that can handle both single and repeatable (non-optional) params.Or expand the type:
export type ParamParser_Generic = - | ParamParser<any, string> - | ParamParser<any, string[]> + | ParamParser<any, string> + | ParamParser<any, string[]> + | ParamParser<any, string | null> + | ParamParser<any, string[] | null>packages/router/src/experimental/route-resolver/matchers/matcher-pattern-path-star.ts (3)
5-5
: Fix typo in JSDoc.“folllowed” → “followed”.
- * Allows matching a static path folllowed by anything. + * Allows matching a static path followed by anything.
22-24
: Preserve original path casing when building; keep lowercase only for matching.Lowercasing
this.path
and reusing it inbuild()
changes the user-supplied prefix casing (e.g. '/Team' becomes '/team'). Keep a lowercase copy for comparisons and the original for output.export class MatcherPatternPathStar implements MatcherPatternPath<{ pathMatch: string }> { - private path: string + private basePath: string + private basePathLower: string constructor(path: string = '') { - this.path = path.toLowerCase() + this.basePath = path + this.basePathLower = path.toLowerCase() } match(path: string): { pathMatch: string } { - if (!path.toLowerCase().startsWith(this.path)) { + if (!path.toLowerCase().startsWith(this.basePathLower)) { throw miss() } return { - pathMatch: path.slice(this.path.length), + pathMatch: path.slice(this.basePathLower.length), } } build(params: { pathMatch: string }): string { - return this.path + params.pathMatch + return this.basePath + params.pathMatch } }Also applies to: 35-37
4-17
: Document the case-insensitive nature of matching.The implementation lowercases both sides; call this out to avoid surprises.
/** - * Allows matching a static path followed by anything. + * Allows matching a static path followed by anything (case-insensitive). + * Matching lowercases both the base path and the input path, but `build()` preserves + * the original casing of the base path.packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts (2)
12-17
: Consider allowing optional inputs in defineParamParser.Today
TIn
excludesnull
, whileParamParser
defaults include it. If you want to author parsers for optional params via this helper, allownull
too.-export function defineParamParser<TOut, TIn extends string | string[]>(parser: { +export function defineParamParser<TOut, TIn extends string | string[] | null>(parser: { get?: (value: TIn) => TOut set?: (value: TOut) => TIn }): ParamParser<TOut, TIn> { return parser }
31-31
: Type-only re-export to avoid value export noise.Re-export
ParamParser
as a type-only export.-export { ParamParser } +export type { ParamParser }packages/router/src/experimental/route-resolver/matchers/param-parsers/numbers.ts (3)
2-2
: Use type-only import for ParamParser.Avoids emitting an unnecessary runtime import.
-import { ParamParser } from './types' +import type { ParamParser } from './types'
32-39
: Place JSDoc immediately before the intended declaration.The doc block appears after the previous const; move it above
PARAM_PARSER_INT
to ensure TypeDoc picks it up correctly.-} satisfies ParamParser<number[] | null, string[] | null> /** - * Native Param parser for integers. - * - * @internal - */ - -export const PARAM_PARSER_INT: ParamParser<number | number[] | null> = { +} satisfies ParamParser<number[] | null, string[] | null> + +/** + * Native Param parser for integers. + * + * @internal + */ +export const PARAM_PARSER_INT: ParamParser<number | number[] | null> = {
6-11
: Minor: redundant finiteness check after integer regex.Given
IS_INTEGER_RE
already restricts to integers,Number(value)
will always be finite. The extraNumber.isFinite(num)
is harmless but unnecessary.No change required; feel free to keep for explicitness.
packages/router/src/experimental/index.ts (1)
42-42
: Type-only re-export for MatchMiss.If
MatchMiss
is a type, re-export it as type-only to avoid value export churn.-export { miss, MatchMiss } from './route-resolver/matchers/errors' +export { miss } from './route-resolver/matchers/errors' +export type { MatchMiss } from './route-resolver/matchers/errors'Confirm
MatchMiss
is a type (or interface). If it’s a value, keep the current value export.packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (3)
164-171
: Fix DEV warning: wrong condition and off-by-one in captured params countThe current check compares object key counts (which are always equal here) and reports
match.length
(includes the full match). This warning will never trigger and misreports counts.Proposed fix:
- Compare the number of capturing groups (
match.length - 1
) with the number of defined params (this.paramsKeys.length
).- Update the message accordingly.
- 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 captured = match.length - 1 + const defined = this.paramsKeys.length + if (captured !== defined) { + warn( + `Regexp matched ${captured} params, but ${defined} params are defined. Found when matching "${path}" against ${String(this.re)}` + ) + } + }
187-189
: Tighten the inferred return type to the specific setter instanceUsing
ReturnType<NonNullable<ParamParser['set']>>
loses the concrete generic instantiation for each param. Basing the return type on the actualparamOptions.set
preserves the preciseTIn
/TOut
pair and improves type safety.- const value: ReturnType<NonNullable<ParamParser['set']>> = ( + const value: ReturnType<NonNullable<typeof paramOptions['set']>> = ( paramOptions.set || identityFn )(params[paramName])- const value: ReturnType<NonNullable<ParamParser['set']>> = ( + const value: ReturnType<NonNullable<typeof paramOptions['set']>> = ( paramOptions.set || identityFn )(params[paramName])Also applies to: 203-205
176-190
: I wasn’t able to confirm howpathParts
is constructed (i.e. that its numeric entries truly represent indexes intothis.paramsKeys
), so I can’t guarantee the safety of droppingparamIndex
. Please review the builder(s) of this class to ensure the numbers inpathParts
are always aligned withObject.keys(this.params)
. If they are, the suggested diff is sound; otherwise, retaining the positional counter avoids introducing subtle mismatches.
📜 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 (8)
packages/router/src/experimental/index.ts
(1 hunks)packages/router/src/experimental/route-resolver/matchers/matcher-pattern-path-star.ts
(1 hunks)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.spec.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)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/router/src/experimental/route-resolver/matchers/matcher-pattern.test-d.ts
- packages/router/src/experimental/route-resolver/matchers/matcher-pattern.spec.ts
🧰 Additional context used
🧠 Learnings (5)
📚 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
📚 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/experimental/index.ts
🧬 Code Graph Analysis (4)
packages/router/src/experimental/route-resolver/matchers/param-parsers/types.ts (2)
packages/router/src/experimental/index.ts (1)
ParamParser
(38-38)packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts (1)
ParamParser
(31-31)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern-path-star.ts (2)
packages/router/src/experimental/index.ts (2)
MatcherPatternPath
(30-30)miss
(42-42)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (1)
MatcherPatternPath
(48-51)
packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts (2)
packages/router/src/experimental/index.ts (2)
defineParamParser
(39-39)ParamParser
(38-38)packages/router/src/experimental/route-resolver/matchers/param-parsers/types.ts (1)
ParamParser
(7-13)
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/matcher-pattern-path-star.ts (1)
match
(26-33)packages/router/src/experimental/route-resolver/resolver-abstract.ts (2)
decode
(154-164)MatcherQueryParams
(124-124)packages/router/src/utils/index.ts (1)
identityFn
(15-15)packages/router/src/warning.ts (1)
warn
(2-9)packages/router/src/encoding.ts (1)
encodeParam
(131-133)
⏰ 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/matchers/param-parsers/types.ts (1)
11-12
: No action needed:NoInfer
is a built-in TS utility type since 5.4TypeScript 5.4 (March 2024) introduced
NoInfer<T>
, so it doesn’t need a manual declaration or import in projects targeting TS 5.4 or later. As long as the router repo’stsconfig.json
uses TS >= 5.4, builds will succeed.packages/router/src/experimental/index.ts (1)
47-52
: Augmentation to discourage children usage looks good.Declaring
children?: never
is an effective compile-time guard.packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (1)
191-194
: Encoding behavior for nullish values is correct as-isCalling
encodeParam
on values coming from(paramOptions.set || identityFn)
is safe:encodeParam(null | undefined)
emits an empty string, and the subsequent.filter(identityFn)
removes empty segments. This matches the intended semantics for optional params and avoids accidental "undefined"/"null" literals in URLs.Also applies to: 207-210
value: string | string[] | null | undefined | ||
) => (value && Array.isArray(value) ? value.map(String) : String(value)) // TODO: `(value an null | undefined)` for types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid serializing null
/undefined
as literal strings.
String(null)
and String(undefined)
yield "null"/"undefined", which is usually incorrect for URLs. Return null
on nullish inputs so upstream can omit the param.
export const PATH_PARAM_DEFAULT_SET = (
value: string | string[] | null | undefined
-) => (value && Array.isArray(value) ? value.map(String) : String(value)) // TODO: `(value an null | undefined)` for types
+) =>
+ value == null
+ ? null
+ : Array.isArray(value)
+ ? value.map(String)
+ : String(value)
If desired, annotate its return type explicitly as string | string[] | null
.
📝 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.
value: string | string[] | null | undefined | |
) => (value && Array.isArray(value) ? value.map(String) : String(value)) // TODO: `(value an null | undefined)` for types | |
export const PATH_PARAM_DEFAULT_SET = ( | |
value: string | string[] | null | undefined | |
) => | |
value == null | |
? null | |
: Array.isArray(value) | |
? value.map(String) | |
: String(value) |
🤖 Prompt for AI Agents
In
packages/router/src/experimental/route-resolver/matchers/param-parsers/index.ts
around lines 23-24, the current parser converts null/undefined to the literal
strings "null"/"undefined"; change the function so that if value is null or
undefined it returns null, otherwise if value is an array return
value.map(String) and otherwise return String(value). Also update the function's
return type annotation to string | string[] | null.
set: (value: number) => String(value), | ||
} satisfies ParamParser<number, string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enforce integer constraints in set().
set()
currently serializes any number, including NaN
, Infinity
, or non-integers like 1.5
, which breaks the contract of an integer parser and the get/set round-trip.
},
- set: (value: number) => String(value),
+ set: (value: number) => {
+ if (!Number.isFinite(value) || !Number.isInteger(value)) throw miss()
+ return String(value)
+ },
} satisfies ParamParser<number, string>
This ensures symmetry with get()
and prevents invalid route construction.
📝 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.
set: (value: number) => String(value), | |
} satisfies ParamParser<number, string> | |
set: (value: number) => { | |
if (!Number.isFinite(value) || !Number.isInteger(value)) throw miss() | |
return String(value) | |
}, | |
} satisfies ParamParser<number, string> |
🤖 Prompt for AI Agents
In
packages/router/src/experimental/route-resolver/matchers/param-parsers/numbers.ts
around lines 14–15, the set() implementation currently serializes any number
(including NaN, Infinity, and non-integers) which violates the integer parser
contract; update set to first validate the input (e.g., ensure
Number.isFinite(value) && Number.isInteger(value)) and throw a clear error (or
assert) when the value is invalid, otherwise return String(value) so get/set
remain symmetric and invalid route values are prevented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/router/src/experimental/route-resolver/matchers/test-utils.ts (4)
1-7
: Consolidate duplicate imports from the same module.Minor cleanup: merge the two imports from './matcher-pattern' into one.
-import { EmptyParams } from './matcher-pattern' -import { - MatcherPatternPath, - MatcherPatternQuery, - MatcherPatternHash, -} from './matcher-pattern' +import { + EmptyParams, + MatcherPatternPath, + MatcherPatternQuery, + MatcherPatternHash, +} from './matcher-pattern'
33-41
: Redundant NaN guard after a numeric-only regex.The
\d+
regex guaranteesmatch[1]
is numeric;Number(match[1])
cannot beNaN
. You can drop the extra check for clarity.- const id = Number(match[1]) - if (Number.isNaN(id)) { - throw miss(`Invalid number: ${String(match[1])}`) - } - return { id } + const id = Number(match[1]) // safe: \d+ guarantees numeric + return { id }Optional: If you want to avoid re-allocating the RegExp per call, you could hoist it:
const USER_ID_RE = /^\/users\/(\d+)$/…and then use
value.match(USER_ID_RE)
.
51-56
: Harden page parsing in PAGE_QUERY_PATTERN_MATCHERTo avoid accepting
'' → 0
,null → 0
, negatives orInfinity
, normalize the raw query value, coerce to a finite integer ≥ 1, and fall back to 1 otherwise.Locations to update:
packages/router/src/experimental/route-resolver/matchers/test-utils.ts
packages/experiments-playground/src/router/index.ts
(mirror the same logic in your demo)Replace in test-utils.ts (lines 51–56):
- const page = Number(query.page) - return { - page: Number.isNaN(page) ? 1 : page, - } + const raw = Array.isArray(query.page) ? query.page[0] : query.page + const n = Number(raw) + return { + page: Number.isFinite(n) && n >= 1 ? Math.trunc(n) : 1, + }And in experiments-playground/src/router/index.ts (around lines 20–24), apply the same change.
You may also want to add unit tests covering:
- Missing or empty
page
- Negative or zero values
- Non-numeric strings
- Arrays of values
Infinity
[optional_refactors_recommended]
59-63
: Move inline comment out of the generic and treat'#'
as no‐hash
- Pull the
// hash could be named anything…
comment out of theMatcherPatternHash<…>
and place it immediately above theexport const ANY_HASH_PATTERN_MATCHER
declaration.- Update the
match
function so that a bare'#'
is treated like “no hash” (returnsnull
), mirroring how an absent fragment is handled.- Leave
build
unchanged (hash ? \
#${hash}` : ''`).Suggested diff in
packages/router/src/experimental/route-resolver/matchers/test-utils.ts
:- export const ANY_HASH_PATTERN_MATCHER: MatcherPatternHash<// hash could be named anything, in this case it creates a param named hash - { hash: string | null }> = { - match: hash => ({ hash: hash ? hash.slice(1) : null }), +// hash could be named anything; this matcher pulls off the leading `#` or returns null if absent +export const ANY_HASH_PATTERN_MATCHER: MatcherPatternHash<{ hash: string | null }> = { + match: hash => { + if (!hash || hash === '#') { + return { hash: null } + } + return { hash: hash.slice(1) } + }, build: ({ hash }) => (hash ? `#${hash}` : ''), }[optional_refactors_recommended]
📜 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 (4)
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/test-utils.ts
(1 hunks)packages/router/vitest.config.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/vitest.config.ts
- packages/router/src/experimental/route-resolver/matchers/param-parsers/numbers.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router/src/experimental/route-resolver/matchers/test-utils.ts (2)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (6)
MatcherPatternPath
(48-51)EmptyParams
(244-244)match
(77-82)match
(145-174)MatcherPatternQuery
(224-226)MatcherPatternHash
(232-234)packages/router/src/experimental/route-resolver/matchers/errors.ts (1)
miss
(19-20)
🔇 Additional comments (2)
packages/router/src/experimental/route-resolver/matchers/test-utils.ts (2)
9-18
: ANY_PATH matcher looks good.Simple and correct round-trip for path passthrough. Helpful for catch-all tests.
20-28
: EMPTY_PATH matcher is correct and aligned with the EmptyParams contract.Cleanly enforces '/' and throws miss() otherwise. Build returns the expected '/'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/router/src/experimental/route-resolver/resolver-static.ts (2)
55-55
: Fix typo: “parend” → “parent”Minor comment typo.
- parent?: EXPERIMENTAL_ResolverRecord | null // the parend can be matchable or not + parent?: EXPERIMENTAL_ResolverRecord | null // the parent can be matchable or not
182-191
: Fix incorrect hash warning condition (only warn when missing leading “#”)Current condition triggers when the hash already starts with '#', suggesting '##'. Invert the predicate.
- if (typeof to === 'object' && to.hash?.startsWith('#')) { + if (typeof to === 'object' && to.hash && !to.hash.startsWith('#')) { warn( `A \`hash\` should always start with the character "#". Replace "${to.hash}" with "#${to.hash}".` ) }packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts (2)
237-246
: Replace ad-hoc cast with proper normalization (remove @ts-expect-error)addMatcher currently fabricates a TMatcherRecord via spread + cast without ensuring required fields (e.g., children array, compiled patterns, score) are populated. This is a footgun for both types and runtime.
Introduce a real normalization step:
- function addMatcher(record: NEW_MatcherRecordRaw, parent?: TMatcherRecord) { - const name = record.name ?? (__DEV__ ? Symbol('unnamed-route') : Symbol()) - // FIXME: proper normalization of the record - // @ts-expect-error: we are not properly normalizing the record yet - const normalizedRecord: TMatcherRecord = { - ...record, - name, - parent, - children: [], - } + function normalizeMatcherRecord( + raw: NEW_MatcherRecordRaw, + parent?: TMatcherRecord + ): TMatcherRecord { + const name = raw.name ?? (__DEV__ ? Symbol('unnamed-route') : Symbol()) + // TODO: compile/validate raw.path/query/hash into MatcherPattern instances if needed + // TODO: compute score if not provided + const normalized = { + name, + path: raw.path, + query: raw.query, + hash: raw.hash, + parent, + children: [] as unknown as TMatcherRecord['children'], + aliasOf: undefined, + group: raw.group, + score: raw.score, + } as TMatcherRecord + return normalized + } + + function addMatcher(record: NEW_MatcherRecordRaw, parent?: TMatcherRecord) { + const normalizedRecord = normalizeMatcherRecord(record, parent) // insert the matcher if it's matchable if (!normalizedRecord.group) { const index = findInsertionIndex(normalizedRecord, matchers) matchers.splice(index, 0, normalizedRecord) // only add the original record to the name map if (normalizedRecord.name && !isAliasRecord(normalizedRecord)) matcherMap.set(normalizedRecord.name, normalizedRecord) // matchers.set(name, normalizedRecord) } record.children?.forEach(childRecord => normalizedRecord.children.push(addMatcher(childRecord, normalizedRecord)) ) return normalizedRecord }Follow-up: if score/path/query/hash need compilation, wire that into normalizeMatcherRecord.
269-276
: Complete removeMatcher: remove from array and clear aliases/childrenThe function doesn’t remove the matcher from the ordered list nor aliases. This can leak stale entries and break matching order.
function removeMatcher(matcher: TMatcherRecord) { matcherMap.delete(matcher.name) - for (const child of matcher.children) { - removeMatcher(child) - } - // TODO: delete from matchers - // TODO: delete children and aliases + // remove children first + for (const child of matcher.children) removeMatcher(child) + // remove from ordered array + const i = matchers.indexOf(matcher) + if (i > -1) matchers.splice(i, 1) + // drop any alias entries pointing to this matcher + for (let j = matchers.length - 1; j >= 0; j--) { + const r = matchers[j] + if (r.aliasOf === matcher) matchers.splice(j, 1) + } + // clear children to avoid retained references + matcher.children.length = 0 }
🧹 Nitpick comments (5)
packages/router/src/experimental/route-resolver/resolver-abstract.ts (2)
96-103
: Consider including absolute-path template literal in MatcherLocationRawMatcherLocationRaw currently comments out the absolute path case. While string already covers it, adding the
/${string}
branch improves intellisense and narrows accidental relative strings passed where absolute is intended.Apply this small tweak:
export type MatcherLocationRaw = - // | `/${string}` | string + | `/${string}` | ResolverLocationAsNamed | ResolverLocationAsPathAbsolute | ResolverLocationAsPathRelative | ResolverLocationAsRelative
147-158
: Make params optional for named locations to improve ergonomics (merge semantics already handle absence)resolve() for named locations merges params with currentLocation?.params. Making params optional avoids boilerplate
{}
for routes that don't need params.export interface ResolverLocationAsNamed { name: RecordName - // FIXME: should this be optional? - params: MatcherParamsFormatted + // Optional: merged with currentLocation?.params during resolution + params?: MatcherParamsFormatted query?: LocationQueryRaw hash?: stringIf you keep it required for now, consider updating the FIXME to a tracked task.
packages/router/src/experimental/route-resolver/resolver-static.ts (1)
200-210
: Avoid passing undefined into Object.assign sourcesflatMap can yield undefined entries when record.query is absent. Object.assign ignores undefined, but filtering improves clarity and type safety.
- const query = Object.assign( + const query = Object.assign( { ...currentLocation?.query, ...normalizeQuery(to.query), }, - ...matched.flatMap(record => - record.query?.map(query => query.build(params)) - ) + ...matched.flatMap(r => (r.query?.map(q => q.build(params)) ?? [])) )packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts (2)
105-167
: Resolve() branch for named/relative is consistent with static; consider adding the same hash warning for parityStatic resolver warns (in dev) when hash lacks a leading '#'. The dynamic resolver doesn’t. Not critical, but parity helps developer DX.
Would you like me to add the same dev-time warning here?
283-289
: Returning internal arrays exposes mutation; prefer a copy or ReadonlygetRecords returns the backing matchers array allowing external mutation that can desync matcherMap.
- function getRecords() { - return matchers - } + function getRecords() { + return matchers.slice() + }Alternatively, type it to ReadonlyArray in the public interface.
📜 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 (7)
packages/router/src/encoding.ts
(3 hunks)packages/router/src/experimental/index.ts
(1 hunks)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts
(1 hunks)packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts
(1 hunks)packages/router/src/experimental/route-resolver/resolver-abstract.ts
(1 hunks)packages/router/src/experimental/route-resolver/resolver-static.spec.ts
(1 hunks)packages/router/src/experimental/route-resolver/resolver-static.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/router/src/experimental/route-resolver/resolver-static.spec.ts
- packages/router/src/encoding.ts
- packages/router/src/experimental/index.ts
- packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts (6)
packages/router/src/experimental/route-resolver/resolver-abstract.ts (7)
RecordName
(10-10)ResolverLocationResolved
(108-125)ResolverLocationAsPathAbsolute
(196-199)ResolverLocationAsPathRelative
(170-183)ResolverLocationAsNamed
(147-158)ResolverLocationAsRelative
(212-225)NO_MATCH_LOCATION
(131-135)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)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (5)
MatcherParamsFormatted
(238-238)MatcherQueryParams
(250-250)MatcherPatternPath
(47-50)MatcherPatternQuery
(223-225)MatcherPatternHash
(231-233)packages/router/src/experimental/route-resolver/resolver-static.ts (2)
buildMatched
(92-102)EXPERIMENTAL_ResolverRecord_Base
(30-58)packages/router/src/matcher/pathParserRanker.ts (1)
comparePathParserScore
(336-363)
packages/router/src/experimental/route-resolver/resolver-abstract.ts (2)
packages/router/src/location.ts (1)
LocationNormalized
(13-18)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (1)
MatcherParamsFormatted
(238-238)
packages/router/src/experimental/route-resolver/resolver-static.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/index.ts (6)
MatcherPatternPath
(26-26)MatcherPatternQuery
(27-27)MatcherPatternHash
(25-25)createStaticResolver
(16-16)MatcherParamsFormatted
(28-28)MatcherQueryParams
(29-29)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (5)
MatcherPatternPath
(47-50)MatcherPatternQuery
(223-225)MatcherPatternHash
(231-233)MatcherParamsFormatted
(238-238)MatcherQueryParams
(250-250)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 (4)
packages/router/src/experimental/route-resolver/resolver-abstract.ts (2)
19-51
: Overload surface looks solid; types align with URL shapesThe resolve() overloads cover absolute, relative (string and object), and named forms coherently. The generic output ResolverLocationResolved keeps name/params/matched available to downstream consumers. No blockers here.
131-136
: NO_MATCH_LOCATION sentinel is well-scoped and type-safeUsing satisfies with Omit ensures LocationNormalized fields must be provided by the caller, avoiding accidental misuse. Good defensive design.
packages/router/src/experimental/route-resolver/resolver-static.ts (1)
221-289
: String/object path resolution path looks correct; fallback to NO_MATCH_LOCATION is consistent
- parseURL handles relative strings with currentLocation?.path.
- For object paths, resolveRelativePath handles basing off currentLocation.
- Matching loop merges path/query/hash params across the matched chain; NO_MATCH_LOCATION is returned when nothing matches.
No functional concerns here.
packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts (1)
312-347
: Insertion index algorithm: tie-breaking by ancestor is reasonable; keep an eye on stabilityThe binary search + ancestor adjustment strategy looks correct with comparePathParserScore. If you encounter instability with deep hierarchies, consider a deterministic tiebreaker (e.g., creation index) as a final key.
I can help craft a focused test that inserts routes with identical scores in different ancestor trees to ensure consistent ordering. Want me to add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/router/src/experimental/index.ts (1)
42-44
: Fix typo in comment (“realying” → “relying”)This typo was previously flagged; reintroducing it hurts polish.
-// this should create type errors if someone is realying on children +// this should create type errors if someone is relying on children
🧹 Nitpick comments (5)
packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts (3)
157-197
: Naming mismatch: describes “strings” but tests object-relative pathsThis block resolves object-relative locations; consider renaming the describe to reflect that it covers objects, not strings.
Apply:
-describe('relative locations as strings', () => { +describe('relative locations as objects', () => {
262-268
: Confirm behavior: object path containing query/hash is treated as pathname-onlyTest clearly asserts the intended semantics. Given this deviates from typical URL parsing (where query/hash would be split), consider adding a comment near the resolver implementation to lock this behavior in, or add a companion test asserting the same for relative object paths.
Do you want me to open a follow-up PR to add a small inline doc in resolver-fixed.ts and a relative-object fullPath assertion?
294-303
: Fix typos in comment (“tests” → “test”, “becuase” → “because”)Minor editorial polish.
- // we don't tests params here becuase it's matcher's responsibility to encode the path + // we don't test params here because it's the matcher's responsibility to encode the pathpackages/router/src/experimental/route-resolver/resolver-fixed.ts (2)
175-178
: Typo in comment: “catched” → “caught”Polish the comment.
- // either one of them must be defined and is catched by the dev only warn above + // either one of them must be defined and is caught by the dev-only warn above
286-287
: Stale TODOObject locations
{ path, query, hash }
are already handled above. Remove the TODO to avoid confusion.- // TODO: handle object location { path, query, hash } + // object locations { path, query, hash } are handled above
📜 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)
packages/experiments-playground/src/router/index.ts
(1 hunks)packages/router/src/encoding.ts
(3 hunks)packages/router/src/experimental/index.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.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/experiments-playground/src/router/index.ts
- packages/router/src/encoding.ts
- packages/router/src/experimental/router.ts
🧰 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/experimental/index.ts
🧬 Code Graph Analysis (1)
packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts (4)
packages/router/src/experimental/index.ts (4)
createFixedResolver
(16-16)MatcherPatternPathStatic
(18-18)MatcherPatternQuery
(27-27)MatcherQueryParams
(29-29)packages/router/src/experimental/route-resolver/resolver-fixed.ts (1)
createFixedResolver
(110-295)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 (7)
packages/router/src/experimental/route-resolver/resolver-fixed.spec.ts (4)
17-56
: Great coverage for path matchersThese tests exercise both static and dynamic path patterns well and assert params formation. No issues spotted.
199-228
: Solid relative string resolutionThese tests nicely validate base-relative resolution for string inputs. No changes requested.
313-333
: Good verification of decoded query handed to matcherThis ensures keys and values are decoded before matching. Looks correct.
354-359
: Hash encoding test is preciseConfirms encoding on output while preserving decoded
hash
in the resolved object. LGTM.packages/router/src/experimental/route-resolver/resolver-fixed.ts (2)
268-277
: No-match merge order looks goodMerging
...url
before...NO_MATCH_LOCATION
ensures sentinel fields win while preserving parsedfullPath
,path
,query
, andhash
. LGTM.
192-209
: Honor explicitto.hash
in named-location resolutionCurrently, the named-route branch always builds the hash from
record.hash.build(params)
and drops any user-providedto.hash
, even though there’s a dev-time warning about hash formatting. To support explicit hashes, preferto.hash
(ensuring it’s prefixed with#
) and fall back to the matcher only when absent:const path = record.path.build(params) - const hash = record.hash?.build(params) ?? '' + // prefer explicit hash, otherwise derive from the record + const hash = + to.hash != null + ? (to.hash.startsWith('#') ? to.hash : '#' + to.hash) + : (record.hash?.build(params) ?? '')• File: packages/router/src/experimental/route-resolver/resolver-fixed.ts (lines ~192–209)
• Dev-only warning block (lines ~183–188): if you choose to keep disallowing explicit hashes, remove or correct that warning to avoid confusion.
[optional_refactors_recommended]packages/router/src/experimental/index.ts (1)
45-50
: Strong compile-time guard:children?: never
Good use of ambient augmentation to surface misuse. This aligns with the earlier guidance to avoid circular imports and keeps the experimental surface tight.
if (typeof to === 'object' && to.hash && to.hash.startsWith('#')) { | ||
warn( | ||
`A \`hash\` should always start with the character "#". Replace "${to.hash}" with "#${to.hash}".` | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Inverted condition and misleading message for hash format warning
The warning triggers when hash
already starts with #
, and suggests prefixing another #
. It should warn when it does not start with #
.
- 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 (typeof to === 'object' && to.hash && !to.hash.startsWith('#')) {
+ warn(
+ `A \`hash\` should start with "#". Replace "${to.hash}" with "#${to.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.
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 (typeof to === 'object' && to.hash && !to.hash.startsWith('#')) { | |
warn( | |
`A \`hash\` should start with "#". Replace "${to.hash}" with "#${to.hash}".` | |
) | |
} |
🤖 Prompt for AI Agents
In packages/router/src/experimental/route-resolver/resolver-fixed.ts around
lines 185 to 189, the condition is inverted and the warning message is
misleading: change the if to trigger when hash does NOT start with '#' (e.g., if
(typeof to === 'object' && to.hash && !to.hash.startsWith('#'))), and update the
message to tell the user to replace e.g. "foo" with "#foo" (keep the same
interpolation style but ensure it suggests prefixing a missing '#').
const query = normalizeQuery(to.query) | ||
url = { | ||
fullPath: NEW_stringifyURL(stringifyQuery, to.path, query, to.hash), | ||
path: resolveRelativePath(to.path, currentLocation?.path || '/'), | ||
query, | ||
hash: to.hash || '', | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relative object locations: fullPath uses unresolved to.path
For object-relative inputs, fullPath
is built from the raw to.path
(e.g., 'foo'
) rather than the resolved absolute path ('/nested/foo'
). This diverges from the string path branch and can leak inconsistent fullPath
values.
- const query = normalizeQuery(to.query)
- url = {
- fullPath: NEW_stringifyURL(stringifyQuery, to.path, query, to.hash),
- path: resolveRelativePath(to.path, currentLocation?.path || '/'),
- query,
- hash: to.hash || '',
- }
+ const query = normalizeQuery(to.query)
+ const resolvedPath = resolveRelativePath(
+ to.path,
+ currentLocation?.path || '/'
+ )
+ url = {
+ fullPath: NEW_stringifyURL(stringifyQuery, resolvedPath, query, to.hash),
+ path: resolvedPath,
+ query,
+ hash: to.hash || '',
+ }
Consider adding/adjusting a test to assert fullPath
for object-relative inputs.
📝 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.
const query = normalizeQuery(to.query) | |
url = { | |
fullPath: NEW_stringifyURL(stringifyQuery, to.path, query, to.hash), | |
path: resolveRelativePath(to.path, currentLocation?.path || '/'), | |
query, | |
hash: to.hash || '', | |
} | |
} | |
const query = normalizeQuery(to.query) | |
const resolvedPath = resolveRelativePath( | |
to.path, | |
currentLocation?.path || '/' | |
) | |
url = { | |
fullPath: NEW_stringifyURL(stringifyQuery, resolvedPath, query, to.hash), | |
path: resolvedPath, | |
query, | |
hash: to.hash || '', | |
} | |
} |
🤖 Prompt for AI Agents
In packages/router/src/experimental/route-resolver/resolver-fixed.ts around
lines 226 to 233, for object-style navigation the fullPath is currently built
from the raw to.path instead of the resolved absolute path; change the code to
first compute the resolved path (resolveRelativePath(to.path,
currentLocation?.path || '/')), use that resolvedPath for both url.path and when
calling NEW_stringifyURL to build fullPath, and keep query/hash as before; also
add or update a test that performs an object-relative navigation (e.g., to: {
path: 'foo', ... } with a nested currentLocation) and asserts that url.fullPath
contains the resolved absolute path (e.g., '/nested/foo') rather than the
unresolved literal.
New version of the internal matcher (renamed as resolver). With more responsibilities and allowing it to be overridden:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores