Custom Route Resolvers #2415
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]
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
packages/router/src/experimental/index.ts (1)
46-46: Fix typo in commentThere's a typo in the comment: "realying" should be "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 (5)
66-80: Lower-casing the stored path drops original casing when buildingThe constructor converts
pathto lowercase andbuild()returns that lowercased value, so/MyTeamturns into/myteamon output. This mutates consumer-supplied paths and may break case-sensitive servers or testing expectations.Store the original string and compare with
toLowerCase()only inmatch():export class MatcherPatternPathStatic implements MatcherPatternPath<EmptyParams> { private path: string constructor(path: string) { - this.path = path.toLowerCase() + this.path = path } match(path: string): EmptyParams { - if (path.toLowerCase() !== this.path) { + if (path.toLowerCase() !== this.path.toLowerCase()) { throw miss() } return {} } build(): string { return this.path } }
104-111:indexOf()allows mid-string matches
'/ateam'.indexOf('/team')returns2, so/ateamerroneously matches the pattern/team*. Usepath.toLowerCase().startsWith(this.path)to ensure the prefix matches from the beginning.match(path: string): { pathMatch: string } { - const pathMatchIndex = path.toLowerCase().indexOf(this.path) - if (pathMatchIndex < 0) { + if (!path.toLowerCase().startsWith(this.path)) { throw miss() } return { - pathMatch: path.slice(pathMatchIndex + this.path.length), + pathMatch: path.slice(this.path.length), } }
218-231: Integer parser rejects "0" and numbers with leading zerosThe regex
/^-?[1-9]\d*$/fails for"0"and numbers with leading zeros like"00", yet those are valid integers for most use cases.If zero should be allowed, update the regex:
-const IS_INTEGER_RE = /^-?[1-9]\d*$/ +const IS_INTEGER_RE = /^-?\d+$/To explicitly reject leading zeros (except for "0" itself), use:
-const IS_INTEGER_RE = /^-?[1-9]\d*$/ +const IS_INTEGER_RE = /^-?(?:0|[1-9]\d*)$/
330-352:build()can emit literal "undefined" when a required param is missingIf
params[paramName]isundefined/null, the fallback identity function passes it straight toencodeParam, which could produce the string"undefined"in the URL. This hides bugs and yields invalid routes.Add validation for required parameters:
build(params: ExtractParamTypeFromOptions<TParamsOptions>): string { let paramIndex = 0 return ( '/' + this.pathParts .map(part => { if (typeof part === 'string') { return part } const paramName = this.paramsKeys[paramIndex++] const paramOptions = this.params[paramName] + const raw = params[paramName] + if (raw == null && !paramOptions.optional) { + throw new Error(`Missing value for param "${String(paramName)}"`) + } const value: ReturnType<NonNullable<Param_GetSet['set']>> = ( paramOptions.set || identityFn - )(params[paramName]) + )(raw) return Array.isArray(value) ? value.map(encodeParam).join('/') : encodeParam(value) }) .filter(identityFn) // filter out empty values .join('/') ) }
416-430: Remove or implement the commented build methodThe build method has a commented implementation. This should either be removed if no longer needed, or properly implemented if it's required functionality.
Would you like me to help implement the build method properly or create an issue to track this?
🧹 Nitpick comments (1)
packages/experiments-playground/src/router/index.ts (1)
40-51: Inconsistent handling of optional query parameterThe type signature indicates
qis optional ({ q?: string }), but thematchmethod always returns a value (empty string when missing). This meansqwill always be present in params even when not in the query string.Consider aligning the implementation with the type:
match: query => { return { - q: typeof query.q === 'string' ? query.q : '', + q: typeof query.q === 'string' ? query.q : undefined, } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/experiments-playground/src/router/index.ts(1 hunks)packages/router/src/experimental/index.ts(1 hunks)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.test-d.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.test-d.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (5)
packages/router/src/experimental/route-resolver/matchers/errors.ts (1)
miss(15-15)packages/router/src/experimental/route-resolver/resolver-abstract.ts (2)
decode(155-165)MatcherQueryParams(125-125)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)
packages/experiments-playground/src/router/index.ts (3)
packages/router/src/experimental/index.ts (7)
EmptyParams(40-40)MatcherPatternHash(36-36)MatcherPatternPathStatic(23-23)MatcherPatternPathCustomParams(25-25)PARAM_PARSER_INTEGER(28-28)experimental_createRouter(1-1)createStaticResolver(16-16)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (5)
EmptyParams(456-456)MatcherPatternHash(444-446)MatcherPatternPathStatic(63-81)MatcherPatternPathCustomParams(272-353)PARAM_PARSER_INTEGER(257-270)packages/router/src/experimental/route-resolver/resolver-static.ts (1)
createStaticResolver(112-295)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/router/src/experimental/index.ts (1)
46-46: 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 children
🧹 Nitpick comments (1)
packages/router/src/experimental/index.ts (1)
26-33: Reduce combinatorial param variants with a builder/factory.Instead of exporting 4 variants per param type, consider a single factory with options (repeatable, optional) to generate the desired spec. Cleaner API, fewer exports, easier to extend.
Example direction (to implement in matcher-pattern, not in this file):
// matcher-pattern.ts export interface ParamOptions { repeatable?: boolean optional?: boolean } export const param = { number: (o: ParamOptions = {}) => makeParam('number', o), int: (o: ParamOptions = {}) => makeParam('int', o), // ... } function makeParam(kind: 'number' | 'int', { repeatable = false, optional = false }: ParamOptions) { // return the proper matcher spec built from options }Then consumers do:
- param.int()
- param.int({ optional: true })
- param.number({ repeatable: true })
- param.number({ repeatable: true, optional: true })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/experiments-playground/src/router/index.ts(1 hunks)packages/router/src/experimental/index.ts(1 hunks)packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/experiments-playground/src/router/index.ts
- packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/router/src/matcher/pathParserRanker.ts (2)
79-80: Fix typo and polish wording in @deprecated JSDocSmall typo + punctuation in the deprecation note.
Apply this diff:
- * @deprecated this option will alsways be `true` in the future. Open a discussion in vuejs/router if you need this to be `false` + * @deprecated This option will always be `true` in the future. Open a discussion in vuejs/router if you need this to be `false`.
375-379: Avoid default-value drift by deriving from BASE_PATH_PARSER_OPTIONS and freezingThese defaults duplicate values already defined in
BASE_PATH_PARSER_OPTIONS. Derive from the single source to prevent divergence over time and make the object readonly at type level.Apply this diff:
-export const PATH_PARSER_OPTIONS_DEFAULTS: PathParserOptions = { - strict: false, - end: true, - sensitive: false, -} +export const PATH_PARSER_OPTIONS_DEFAULTS = { + strict: BASE_PATH_PARSER_OPTIONS.strict, + end: BASE_PATH_PARSER_OPTIONS.end, + sensitive: BASE_PATH_PARSER_OPTIONS.sensitive, +} as const satisfies PathParserOptions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router/src/experimental/index.ts(1 hunks)packages/router/src/matcher/pathParserRanker.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router/src/experimental/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router/src/matcher/pathParserRanker.ts (1)
packages/router/src/matcher/index.ts (1)
PathParserOptions(635-635)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
packages/router/src/matcher/pathParserRanker.ts (1)
336-339: comparePathParserScore signature change is safe—call sites verifiedAll invocations of
comparePathParserScorestill type-check correctly:
- Tests (
packages/router/__tests__/matcher/pathRanking.spec.ts): passing{ score: number[][] }- Core matcher (
packages/router/src/matcher/index.ts): passing fullPathParserobjects (structurally compatible)- Experimental resolver (
packages/router/src/experimental/route-resolver/resolver-dynamic.ts): same as aboveNo stale types found. The helper
isLastScoreNegativealready consumesPathParser['score'](akanumber[][]), so migrating it to a standalonenumber[][]ortype Score = number[][]is purely optional and can be deferred.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (6)
416-430: Remove the commented-out build() methodDead commented code adds noise. Either implement it or delete; given
buildis provided via constructor, removing these lines is appropriate.- // build(params: TParams): string { - // let path = this.re.source - // for (const param of this.params) { - // const value = params[param.name as keyof TParams] - // if (value == null) { - // throw new Error(`Matcher build: missing param ${param.name}`) - // } - // path = path.replace( - // /([^\\]|^)\([^?]*\)/, - // `$1${encodeParam(param.set(value))}` - // ) - // } - // return path - // }
66-80: Preserve original casing; only lowercase during matchingLowercasing the stored path mutates consumer input and causes
build()to emit a lowercased path. Store both the original and a lowercased copy; use the lowercased copy only inmatch(), and return the original inbuild().- private path: string + private pathOriginal: string + private pathLC: string constructor(path: string) { - this.path = path.toLowerCase() + this.pathOriginal = path + this.pathLC = path.toLowerCase() } match(path: string): EmptyParams { - if (path.toLowerCase() !== this.path) { + if (path.toLowerCase() !== this.pathLC) { throw miss() } return {} } build(): string { - return this.path + return this.pathOriginal }
99-116: Anchor star prefix match to start and preserve original casing
indexOf()allows mid-string matches (e.g.,/ateammatches/team*). UsestartsWith()to anchor at the beginning.- Also avoid lowercasing the built value; store original and lowercased variants similarly to Static.
-export class MatcherPatternPathStar - implements MatcherPatternPath<{ pathMatch: string }> -{ - private path: string - constructor(path: string = '') { - this.path = path.toLowerCase() - } +export class MatcherPatternPathStar + implements MatcherPatternPath<{ pathMatch: string }> +{ + private pathOriginal: string + private pathLC: string + constructor(path: string = '') { + this.pathOriginal = path + this.pathLC = path.toLowerCase() + } match(path: string): { pathMatch: string } { - const pathMatchIndex = path.toLowerCase().indexOf(this.path) - if (pathMatchIndex < 0) { + const lower = path.toLowerCase() + if (!lower.startsWith(this.pathLC)) { throw miss() } return { - pathMatch: path.slice(pathMatchIndex + this.path.length), + pathMatch: path.slice(this.pathOriginal.length), } } build(params: { pathMatch: string }): string { - return this.path + params.pathMatch + return this.pathOriginal + params.pathMatch }
218-232: Integer regex rejects “0” and numbers with leading zero
/^-?[1-9]\d*$/excludes"0"and"00". If the intention is to accept any integer (including 0), use/^-?\d+$/. If leading zeros are undesired, add an additional guard.-const IS_INTEGER_RE = /^-?[1-9]\d*$/ +const IS_INTEGER_RE = /^-?\d+$/
339-347: Throw on missing required params in build() to avoid silently emitting malformed URLsAbsent values currently become empty segments, hiding bugs. If a param isn’t marked optional, throw.
- const paramName = this.paramsKeys[paramIndex++] - const paramOptions = this.params[paramName] - const value: ReturnType<NonNullable<ParamParser['set']>> = ( - paramOptions.set || identityFn - )(params[paramName]) + const paramName = this.paramsKeys[paramIndex++] + const paramOptions = this.params[paramName] + const raw = params[paramName as keyof typeof params] + if (raw == null && !paramOptions.optional) { + throw new Error( + `Missing value for param "${String(paramName)}" when building path` + ) + } + const value: ReturnType<NonNullable<ParamParser['set']>> = ( + paramOptions.set || identityFn + )(raw as any)
199-203: Expose optional flag on custom param options to enable safe build-time checks
build()currently cannot distinguish missing required params from intentionally omitted optional ones. Addoptional?: booleanto options so builders can validate.interface MatcherPatternPathCustomParamOptions< TIn extends string | string[] | null = string | string[] | null, TOut = string | string[] | null, > extends ParamParser<TOut, TIn> { repeat?: boolean - // NOTE: not needed because in the regexp, the value is undefined if - // the group is optional and not given - // optional?: boolean + optional?: boolean }
🧹 Nitpick comments (5)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (5)
84-84: Typo: “folllowed” → “followed”- * Allows matching a static path folllowed by anything. + * Allows matching a static path followed by anything.
143-146: Allow null in defineParamParser input type
ParamParsersupportsTInincludingnull.defineParamParsershould mirror that to avoid friction when declaring optional parsers.-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> {
318-325: Fix param count warning: off-by-one and confusing message
match.lengthincludes the whole match at index 0. Report captured groups withmatch.length - 1and compare to the number of defined params for clarity.- 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 expected = this.paramsKeys.length + const captured = match.length - 1 + if (captured !== expected) { + warn( + `Regexp captured ${captured} params, but ${expected} are defined. When matching "${path}" against ${String( + this.re + )}` + ) + } + }
385-386: JSDoc nit: “patch” → “path”- * @throws if the patch doesn't match + * @throws if the path doesn't match
408-413: Fix param count warning: off-by-one and clearer wordingFor dynamic matcher too, account for the entire match at index 0.
- if (__DEV__ && i !== match.length) { - warn( - `Regexp matched ${match.length} params, but ${i} params are defined. Found when matching "${path}" against ${String(this.re)}` - ) - } + if (__DEV__) { + const expected = Object.keys(this.params).length + const captured = match.length - 1 + if (captured !== expected) { + warn( + `Regexp captured ${captured} params, but ${expected} are defined. When matching "${path}" against ${String( + this.re + )}` + ) + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router/src/experimental/index.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/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (5)
packages/router/src/experimental/route-resolver/matchers/errors.ts (1)
miss(15-15)packages/router/src/experimental/route-resolver/resolver-abstract.ts (2)
decode(155-165)MatcherQueryParams(125-125)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 (1)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (1)
187-194: Type-level mapping from parsers to params is clean and pragmaticThe
ParamsFromParsersconditional type sensibly falls back toTInwhenTOutisunknown, making API ergonomics better.
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts
Show resolved
Hide resolved
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
packages/router/src/experimental/router.ts (4)
675-703: Address remaining type gaps in resolve() instead of suppressingThere were multiple
@ts-expect-errorentries here. The proposed overloads + adapter above remove them. If any remain, prefer precise casts or adapter types over@ts-expect-error.
720-724: Overload push()/replace() to match public API while keeping relative-arg supportCurrent arrow functions accept
_resolveArgsonly, forcing@ts-expect-errorlater. Overload them to acceptRouteLocationRaw(as required by the public API) and your extended_resolveArgs.Apply this diff:
- const push = (...args: _resolveArgs) => pushWithRedirect(resolve(...args)) - - const replace = (...args: _resolveArgs) => - pushWithRedirect(resolve(...args), true) + function push(to: RouteLocationRaw): Promise<NavigationFailure | void | undefined> + function push(...args: _resolveArgs): Promise<NavigationFailure | void | undefined> + function push(...args: any[]): Promise<NavigationFailure | void | undefined> { + return pushWithRedirect(resolve(...(args as _resolveArgs))) + } + + function replace(to: RouteLocationRaw): Promise<NavigationFailure | void | undefined> + function replace(...args: _resolveArgs): Promise<NavigationFailure | void | undefined> + function replace(...args: any[]): Promise<NavigationFailure | void | undefined> { + return pushWithRedirect(resolve(...(args as _resolveArgs)), true) + }
735-773: Avoid mutating a possibly string-derived redirect; build a typed RouteLocationRawThe current code mutates
newTargetLocation.paramsafter converting from a string, requiring a@ts-expect-error. Build aRouteLocationRawinstead and use it downstream.Apply this diff:
- if (typeof newTargetLocation === 'string') { - newTargetLocation = - newTargetLocation.includes('?') || newTargetLocation.includes('#') - ? (newTargetLocation = locationAsObject(newTargetLocation)) - : // force empty params - { path: newTargetLocation } - // @ts-expect-error: force empty params when a string is passed to let - // the router parse them again - newTargetLocation.params = {} - } + let redirectLocation: RouteLocationRaw + if (typeof newTargetLocation === 'string') { + redirectLocation = + newTargetLocation.includes('?') || newTargetLocation.includes('#') + ? locationAsObject(newTargetLocation) + : { path: newTargetLocation } + // force empty params when redirecting by path + if ('path' in redirectLocation) { + redirectLocation = assign({ params: {} }, redirectLocation) + } + } else { + redirectLocation = newTargetLocation + } @@ - if ( + if ( __DEV__ && - newTargetLocation.path == null && - !('name' in newTargetLocation) + (redirectLocation as any).path == null && + !('name' in (redirectLocation as any)) ) { warn( `Invalid redirect found:\n${JSON.stringify( - newTargetLocation, + redirectLocation, null, 2 )}\n when navigating to "${ to.fullPath }". A redirect must contain a name or path. This will break in production.` ) throw new Error('Invalid redirect') } @@ - return assign( + return assign( { query: to.query, hash: to.hash, // avoid transferring params if the redirect has a path - params: newTargetLocation.path != null ? {} : to.params, + params: (redirectLocation as any).path != null ? {} : to.params, }, - newTargetLocation + redirectLocation )
651-703: Remove @ts-expect-error in resolve() via overloads and a typed adapterAdd explicit overloads to match EXPERIMENTAL_Router_Base.resolve() and adapt the resolver’s return type to the router shape to avoid the
matchedtype mismatch.Apply this diff:
- function resolve( - ...[to, currentLocation]: _resolveArgs - ): RouteLocationResolved { + // Overloads to align with EXPERIMENTAL_Router_Base signatures + function resolve<Name extends keyof RouteMap = keyof RouteMap>( + to: RouteLocationAsRelativeTyped<RouteMap, Name>, + currentLocation?: RouteLocationNormalizedLoaded + ): RouteLocationResolved<Name> + function resolve( + to: RouteLocationAsString | RouteLocationAsRelative | RouteLocationAsPath, + currentLocation?: RouteLocationNormalizedLoaded + ): RouteLocationResolved + function resolve( + ...[to, currentLocation]: _resolveArgs + ): RouteLocationResolved { @@ - const matchedRoute = resolver.resolve( - // @ts-expect-error FIXME: incompatible types - to, - // FIXME: incompatible `matched` requires casting - currentLocation - ) + // Adapter to align resolver result with router result + type ResolvedAdapter = Omit< + ResolverLocationResolved<TRecord>, + 'matched' + > & { + matched: EXPERIMENTAL_RouteRecordNormalized[] + } + const matchedRoute = resolver.resolve( + to as any, + currentLocation as any + ) as ResolvedAdapter @@ - // matchedRoute is always a new object - // @ts-expect-error: FIXME: the `matched` property is different return assign(matchedRoute, { redirectedFrom: undefined, href, meta: mergeMetaFields(matchedRoute.matched), })
🧹 Nitpick comments (4)
packages/router/src/experimental/router.ts (4)
1341-1354: Clean up ts-expect-error on resolve/push/replace after overload fixesWith the added overloads and adapter in resolve(), plus overloaded push()/replace(), these suppressions should no longer be necessary.
Apply this diff:
getRoutes, - // @ts-expect-error FIXME: update EXPERIMENTAL_Router types resolve, options, - // @ts-expect-error FIXME: update EXPERIMENTAL_Router types push, - // @ts-expect-error FIXME: update EXPERIMENTAL_Router types replace,
742-744: Prefer not mutating incoming redirect objectsEven after the refactor, avoid mutating user-supplied objects. Always create a new object when adding
params,query, orhash.
1280-1307: isReady()/markAsReady(): ensure idempotency and listener setup order is intentionalBehavior looks correct. One nit: document the guarantee that listeners are set up at most once even on error paths, and that errors mark the router as not-ready.
606-607: hasRoute(): check name normalization/parsing consistency with resolverIf the resolver normalizes names differently (e.g., Symbols, or string case), ensure
resolver.getRecord(name)follows the same convention asaddRoute/matching. Add a small doc or narrow the type if needed.
📜 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/index.ts(1 hunks)packages/router/src/experimental/router.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router/src/experimental/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router/src/experimental/router.ts (8)
packages/router/src/matcher/pathParserRanker.ts (1)
PathParserOptions(86-89)packages/router/src/scrollBehavior.ts (7)
RouterScrollBehavior(35-46)saveScrollPosition(180-185)getScrollKey(173-176)computeScrollPosition(90-93)_ScrollPositionNormalized(26-30)getSavedScrollPosition(187-192)scrollToPosition(95-171)packages/router/src/types/index.ts (3)
RouteMeta(260-260)RawRouteComponent(186-186)Lazy(15-15)packages/router/src/errors.ts (9)
NavigationFailure(58-74)createRouterError(124-148)ErrorTypes(89-95)ErrorTypes(96-103)ErrorTypes(104-106)ErrorTypes(107-109)ErrorTypes(110-112)NavigationRedirectError(81-85)isNavigationFailure(184-193)packages/router/src/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/navigationGuards.ts (3)
extractChangingRecords(417-447)extractComponentsGuards(234-354)guardToPromiseFn(123-213)
⏰ 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
| // @ts-expect-error: FIXME: refactor with new types once it's possible | ||
| app.config.globalProperties.$router = router | ||
| Object.defineProperty(app.config.globalProperties, '$route', { | ||
| enumerable: true, | ||
| get: () => unref(currentRoute), | ||
| }) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Global properties typing: keep suppression or augment types; do not ship with FIXME
This @ts-expect-error stems from $router being typed as the stable Router. Either:
- Module-augment
ComponentCustomPropertiesfor the experimental router inexperimental/index.ts, or - Cast at the injection site with a well-documented adapter type.
Run this to confirm augmentation exists and matches EXPERIMENTAL_Router:
🏁 Script executed:
#!/bin/bash
# Look for module augmentation exposing EXPERIMENTAL_Router on globalProperties
rg -n "declare module ['\"]vue['\"]" -A 20 -g packages/router/src/experimental/index.ts
rg -n "\$router" packages/router/src/experimental -A 2 -B 2Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "--- Searching for `declare module 'vue'` ---"
rg -n "declare module ['\"]vue['\"]" || true
echo "--- Searching for ComponentCustomProperties ---"
rg -n "ComponentCustomProperties" || true
echo "--- Searching for EXPERIMENTAL_Router ---"
rg -n "EXPERIMENTAL_Router" || true
echo "--- Searching for globalProperties.*\\$router ---"
rg -n "globalProperties.*\\$router" -S || true
echo "--- Searching for \\$router in packages/router ---"
rg -n "\\$router" packages/router -S || true
echo "--- Listing files under packages/router/src/experimental if any ---"
rg -n ".*" packages/router/src/experimental -S || true
echo "--- Show packages/router/src/experimental/index.ts if present ---"
if [ -f packages/router/src/experimental/index.ts ]; then
sed -n '1,240p' packages/router/src/experimental/index.ts || true
else
echo "file not found: packages/router/src/experimental/index.ts"
fiLength of output: 3572
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "--- show packages/router/src/globalExtensions.ts ---"
if [ -f packages/router/src/globalExtensions.ts ]; then
sed -n '1,240p' packages/router/src/globalExtensions.ts || true
else
echo "file not found: packages/router/src/globalExtensions.ts"
fi
echo "--- show packages/router/src/experimental/index.ts ---"
if [ -f packages/router/src/experimental/index.ts ]; then
sed -n '1,240p' packages/router/src/experimental/index.ts || true
else
echo "file not found: packages/router/src/experimental/index.ts"
fi
echo "--- show packages/router/src/experimental/router.ts (1300-1380) ---"
if [ -f packages/router/src/experimental/router.ts ]; then
sed -n '1300,1380p' packages/router/src/experimental/router.ts || true
else
echo "file not found: packages/router/src/experimental/router.ts"
fiLength of output: 7667
Global properties typing: add augmentation or cast — do not ship with @ts-expect-error
I checked the repo: packages/router/src/globalExtensions.ts currently augments 'vue' and types ComponentCustomProperties.$router as the stable Router, and there is no module augmentation for EXPERIMENTAL_Router in packages/router/src/experimental/index.ts. That explains the @ts-expect-error at the install site in the experimental router.
Files to fix
- packages/router/src/globalExtensions.ts — declares ComponentCustomProperties.$router as Router (stable).
- packages/router/src/experimental/router.ts — install(app: App) assigns app.config.globalProperties.$router = router (this line has the
@ts-expect-error). - packages/router/src/experimental/index.ts — currently no 'vue' augmentation for EXPERIMENTAL_Router (add it here) or use a cast at the injection site.
Recommended fixes (pick one)
- Module augmentation (preferred): add an augmentation in packages/router/src/experimental/index.ts to expose EXPERIMENTAL_Router on globalProperties so the assignment is typed correctly. Example:
// packages/router/src/experimental/index.ts declare module 'vue' { export interface ComponentCustomProperties { $router: EXPERIMENTAL_Router // add $route here too if the experimental route type differs } } - Narrow cast with a documented adapter type (if augmentation isn’t desirable): replace the assignment with a safe cast at the injection site and document why:
// packages/router/src/experimental/router.ts (install) type RouterAdapter = { $router: EXPERIMENTAL_Router } & Record<string, unknown> ;(app.config.globalProperties as unknown as RouterAdapter).$router = router
Do not leave the @ts-expect-error in shipped code — either augment ComponentCustomProperties for the experimental router or cast with a clear, documented adapter type so typing is intentional and auditable.
There was a problem hiding this comment.
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 --runin 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.
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_BaseThe 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-errorcomments. 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_REDIRECTbranch, the code usesshouldRedirect(from line 787) instead offailure.towhen 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-erroron line 1367 indicates a type mismatch betweenEXPERIMENTAL_Routerand the expectedRoutertype 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_MatcherRecordRawwith 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 tocompileRouteRecordfunction.The function returns
NEW_MatcherRecordRawbut 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
.todowhich 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.todoblock 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-errorcomment 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
instancesproperty 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
runWithContextfunction 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 inisMatchablefunction.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-errordirectives. 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
mergeMetaFieldsfunction provides a simple and effective way to merge meta fields from matched records using object spread.
| // FIXME: is this a ts bug? | ||
| // @ts-expect-error | ||
| fromLocation | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Remove unnecessary type assertion comments.
The FIXME comments and @ts-expect-error directives indicate type issues that should be properly resolved rather than suppressed.
🏁 Script executed:
#!/bin/bash
# Check if there are type definition issues with the matcher.resolve method
ast-grep --pattern 'matcher.resolve($_)' packages/router/src/experimental/route-resolver/matcher-resolve.spec.ts | head -20Length of output: 428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='packages/router/src/experimental/route-resolver/matcher-resolve.spec.ts'
echo "== lines 190-230 =="
sed -n '190,230p' "$file"
echo
echo "== lines 380-420 =="
sed -n '380,420p' "$file"
echo
echo "== lines 500-520 =="
sed -n '500,520p' "$file"
echo
echo "== occurrences of @ts-expect-error in file =="
rg -n '@ts-expect-error' "$file" || trueLength of output: 3829
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find createCompiledMatcher =="
rg -n "createCompiledMatcher" || true
echo
echo "== find isMatcherLocationResolved =="
rg -n "isMatcherLocationResolved" || true
echo
echo "== find matcher.resolve (symbol usages) =="
rg -n "matcher.resolve" || true
echo
echo "== find definitions of resolve methods/functions =="
rg -n --hidden --line-number --no-heading "resolve(" packages | sed -n '1,200p' || true
echo
echo "== search for types: MatcherLocation, MatcherLocationResolved, START_LOCATION =="
rg -n "MatcherLocation|MatcherLocationResolved|START_LOCATION" || true
echo
echo "== show file(s) where createCompiledMatcher is defined (first match) =="
file=$(rg -n "createCompiledMatcher" -S --hidden --line-number --no-heading | sed -n '1p' | cut -d: -f1 || true)
if [ -n "$file" ]; then
echo "== $file (lines 1-240) =="
sed -n '1,240p' "$file"
fi
echo
echo "== show potential matcher implementation files containing 'resolve' definition (first few matches) =="
rg -n "class .*Matcher|interface .*Matcher|export .*resolve\(" packages | sed -n '1,200p' || trueLength of output: 35661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== show resolver-dynamic.ts around createCompiledMatcher =="
file='packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts'
rg -n "createCompiledMatcher" "$file" || true
sed -n '1,240p' "$file"
echo
echo "== show later part of resolver-dynamic.ts (where resolve is implemented) =="
sed -n '240,640p' "$file"
echo
echo "== show resolver-abstract.ts types referenced in the test =="
sed -n '1,240p' 'packages/router/src/experimental/route-resolver/resolver-abstract.ts'Length of output: 22925
Replace @ts-expect-error with explicit narrowing/casts in matcher-resolve.spec.ts
The two @ts-expect-error suppressions are hiding overload/narrowing issues — don't remove them without fixing the types. Prefer explicit narrowing or a small refactor so the intent and allowed types are clear.
-
File: packages/router/src/experimental/route-resolver/matcher-resolve.spec.ts (around the resolvedFrom ternary, comment lines ~209–213)
- Replace the @ts-expect-error usage with an explicit cast to the expected raw-location union:
// before const resolvedFrom = isMatcherLocationResolved(fromLocation) ? fromLocation : matcher.resolve( // FIXME: is this a ts bug? // @ts-expect-error fromLocation ) // after const resolvedFrom = isMatcherLocationResolved(fromLocation) ? fromLocation : matcher.resolve( fromLocation as ResolverLocationAsPathAbsolute | ResolverLocationAsNamed | `/${string}` ) -
File: packages/router/src/experimental/route-resolver/matcher-resolve.spec.ts (around the matcher.resolve(toLocation, ...) call, comment lines ~217–222)
- Avoid @ts-expect-error by making the call explicit or by splitting the current location into a variable so overload resolution is clearer:
// before const result = matcher.resolve( // FIXME: should work now // @ts-expect-error toLocation, resolvedFrom === START_LOCATION ? undefined : resolvedFrom ) // after (option A: explicit casts) const result = matcher.resolve( toLocation as Exclude<MatcherLocationRaw, string> | `/${string}`, resolvedFrom === START_LOCATION ? undefined : (resolvedFrom as ResolverLocationResolved<NEW_MatcherRecord>) ) // after (option B: clearer refactor) const current = resolvedFrom === START_LOCATION ? undefined : resolvedFrom const result = matcher.resolve(toLocation as Exclude<MatcherLocationRaw, string> | `/${string}`, current)
Why: explicit casts or a tiny refactor document the expected types and avoid blanket @ts-expect-error suppressions that hide real typing problems. If you prefer, cast to Parameters<Matcher['resolve']>[0] / [1] instead to tie the assertions to the actual overloads.
Please apply one of the above fixes (casts or the small refactor) and remove the @ts-expect-error comments once TypeScript is satisfied.
🤖 Prompt for AI Agents
In packages/router/src/experimental/route-resolver/matcher-resolve.spec.ts
around lines ~209–222, remove the two @ts-expect-error comments and replace them
with explicit narrowing/casts or a tiny refactor so overload resolution is
clear: cast fromLocation when calling matcher.resolve to the expected
raw-location union (e.g. ResolverLocationAsPathAbsolute |
ResolverLocationAsNamed | `/${string}`) instead of suppressing errors, and for
the second call either cast to the exact parameter types (e.g.
Exclude<MatcherLocationRaw, string> | `/${string>` for the first arg and
ResolverLocationResolved<NEW_MatcherRecord> for the second) or assign
resolvedFrom === START_LOCATION ? undefined : resolvedFrom to a local variable
(current) and pass that variable so TypeScript infers the correct overload;
alternatively you may use Parameters<Matcher['resolve']>[0]/[1] for tied
casts—remove the @ts-expect-error lines once the explicit casts/refactor satisfy
the compiler.
|
|
||
| it.todo('discards non existent params', () => { | ||
| assertRecordMatch( | ||
| { path: '/', name: 'home', components }, | ||
| { name: 'home', params: { a: 'a', b: 'b' } }, | ||
| { name: 'home', path: '/', params: {} } | ||
| ) | ||
| expect('invalid param(s) "a", "b" ').toHaveBeenWarned() | ||
| assertRecordMatch( | ||
| { path: '/:b', name: 'a', components }, | ||
| { name: 'a', params: { a: 'a', b: 'b' } }, | ||
| { name: 'a', path: '/b', params: { b: 'b' } } | ||
| ) | ||
| expect('invalid param(s) "a"').toHaveBeenWarned() | ||
| }) | ||
|
|
There was a problem hiding this comment.
Fix inconsistent test expectations in "discards non existent params" test.
The test is marked as .todo but contains implementation. Additionally, the warning expectation on line 468 has a trailing space in the string that may cause the assertion to fail.
- it.todo('discards non existent params', () => {
+ it('discards non existent params', () => {
assertRecordMatch(
{ path: '/', name: 'home', components },
{ name: 'home', params: { a: 'a', b: 'b' } },
{ name: 'home', path: '/', params: {} }
)
- expect('invalid param(s) "a", "b" ').toHaveBeenWarned()
+ expect('invalid param(s) "a", "b"').toHaveBeenWarned()
assertRecordMatch(
{ path: '/:b', name: 'a', components },
{ name: 'a', params: { a: 'a', b: 'b' } },
{ name: 'a', path: '/b', params: { b: 'b' } }
)
expect('invalid param(s) "a"').toHaveBeenWarned()
})📝 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.
| it.todo('discards non existent params', () => { | |
| assertRecordMatch( | |
| { path: '/', name: 'home', components }, | |
| { name: 'home', params: { a: 'a', b: 'b' } }, | |
| { name: 'home', path: '/', params: {} } | |
| ) | |
| expect('invalid param(s) "a", "b" ').toHaveBeenWarned() | |
| assertRecordMatch( | |
| { path: '/:b', name: 'a', components }, | |
| { name: 'a', params: { a: 'a', b: 'b' } }, | |
| { name: 'a', path: '/b', params: { b: 'b' } } | |
| ) | |
| expect('invalid param(s) "a"').toHaveBeenWarned() | |
| }) | |
| it('discards non existent params', () => { | |
| assertRecordMatch( | |
| { path: '/', name: 'home', components }, | |
| { name: 'home', params: { a: 'a', b: 'b' } }, | |
| { name: 'home', path: '/', params: {} } | |
| ) | |
| expect('invalid param(s) "a", "b"').toHaveBeenWarned() | |
| assertRecordMatch( | |
| { path: '/:b', name: 'a', components }, | |
| { name: 'a', params: { a: 'a', b: 'b' } }, | |
| { name: 'a', path: '/b', params: { b: 'b' } } | |
| ) | |
| expect('invalid param(s) "a"').toHaveBeenWarned() | |
| }) |
🤖 Prompt for AI Agents
In packages/router/src/experimental/route-resolver/matcher-resolve.spec.ts
around lines 461 to 476, the test labeled it.todo contains actual assertions and
has an inconsistent expectation string with a trailing space that may break the
warning assertion; change it.todo to it (enable the test) or remove the test
body if it should remain todo, and remove the trailing space from the expect
string on line 468 so the expected warning exactly matches the emitted message
(also verify the second expect string matches exactly and adjust if needed).
| 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.
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.
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.
| }) | ||
|
|
||
| it('decodes query from a string', () => { | ||
| expect(matcher.resolve('/foo?foo=%23%2F%3F')).toMatchObject({ |
There was a problem hiding this comment.
Fix string location resolution call.
The test uses matcher.resolve('/foo?foo=%23%2F%3F') directly as a string, but based on the resolver interface, it should be wrapped in an object with a path property.
- expect(matcher.resolve('/foo?foo=%23%2F%3F')).toMatchObject({
+ expect(matcher.resolve({ path: '/foo?foo=%23%2F%3F' })).toMatchObject({📝 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.
| expect(matcher.resolve('/foo?foo=%23%2F%3F')).toMatchObject({ | |
| expect(matcher.resolve({ path: '/foo?foo=%23%2F%3F' })).toMatchObject({ |
🤖 Prompt for AI Agents
In packages/router/src/experimental/route-resolver/old/resolver-dynamic.spec.ts
around line 288, the test incorrectly calls matcher.resolve with a raw string;
update the call to pass an object with a path property (e.g., matcher.resolve({
path: '/foo?foo=%23%2F%3F' })) so it matches the resolver interface expecting an
object containing path.
| }) | ||
|
|
||
| it('decodes hash from a string', () => { | ||
| expect(matcher.resolve('/foo#%22')).toMatchObject({ |
There was a problem hiding this comment.
Fix string location resolution call.
Similar to the previous issue, the string should be wrapped in an object.
- expect(matcher.resolve('/foo#%22')).toMatchObject({
+ expect(matcher.resolve({ path: '/foo#%22' })).toMatchObject({📝 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.
| expect(matcher.resolve('/foo#%22')).toMatchObject({ | |
| expect(matcher.resolve({ path: '/foo#%22' })).toMatchObject({ |
🤖 Prompt for AI Agents
In packages/router/src/experimental/route-resolver/old/resolver-dynamic.spec.ts
around line 296, the test calls matcher.resolve('/foo#%22') with a raw string
but the resolver expects a location object; update the call to pass an object
containing the string (e.g., { pathname: '/foo#%22' } or the correct location
key used elsewhere in the tests) so the location is wrapped in an object
matching the resolver's API, then run the spec to ensure the matcher returns the
expected object.
| 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.
💡 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.tsLength 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" || trueLength 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.
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.
🛠️ 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.
| // @ts-expect-error FIXME: update EXPERIMENTAL_Router types | ||
| resolve, | ||
| options, | ||
|
|
||
| // @ts-expect-error FIXME: update EXPERIMENTAL_Router types | ||
| push, | ||
| // @ts-expect-error FIXME: update EXPERIMENTAL_Router types | ||
| replace, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Track TypeScript errors for resolution
Multiple @ts-expect-error comments indicate type mismatches that need to be resolved before this experimental API stabilizes.
Let me verify if there are other TypeScript errors in the experimental module that need tracking:
🏁 Script executed:
#!/bin/bash
# Find all @ts-expect-error comments in experimental router code
echo "=== TypeScript errors needing resolution ==="
rg "@ts-expect-error" packages/router/src/experimental/ -n --no-heading | grep -v "test" | head -20Length of output: 2532
Fix TypeScript errors in experimental router (required before stabilizing API)
Multiple remaining @ts-expect-error occurrences in the experimental router indicate unresolved type mismatches that must be fixed.
Affected locations:
- packages/router/src/experimental/router.ts:676 — // @ts-expect-error FIXME: incompatible types
- packages/router/src/experimental/router.ts:697 — // @ts-expect-error: FIXME: the
matchedproperty is different - packages/router/src/experimental/router.ts:741 — // @ts-expect-error: force empty params when a string is passed to let
- packages/router/src/experimental/router.ts:792 — // @ts-expect-error: FIXME: refactor location types
- packages/router/src/experimental/router.ts:850 — // @ts-expect-error: FIXME: failure.to should not contain relative locations
- packages/router/src/experimental/router.ts:856 — // @ts-expect-error: added only in dev
- packages/router/src/experimental/router.ts:858 — ? // @ts-expect-error
- packages/router/src/experimental/router.ts:872 — // @ts-expect-error: FIXME: refactor location types
- packages/router/src/experimental/router.ts:1120 — // @ts-expect-error: FIXME: refactor location types
- packages/router/src/experimental/router.ts:1167 — // @ts-expect-error: to should be an absolute location
- packages/router/src/experimental/router.ts:1343 — // @ts-expect-error FIXME: update EXPERIMENTAL_Router types
- packages/router/src/experimental/router.ts:1347 — // @ts-expect-error FIXME: update EXPERIMENTAL_Router types
- packages/router/src/experimental/router.ts:1349 — // @ts-expect-error FIXME: update EXPERIMENTAL_Router types
- packages/router/src/experimental/router.ts:1367 — // @ts-expect-error: FIXME: refactor with new types once it's possible
- packages/router/src/experimental/router.ts:1399 — // @ts-expect-error: FIXME: refactor with new types once it's possible
- packages/router/src/experimental/router.ts:1421 — // @ts-expect-error: FIXME: refactor with new types once it's possible
- packages/router/src/experimental/route-resolver/resolver-abstract.ts:189 — // @ts-expect-error: overload are not correctly identified
- packages/router/src/experimental/route-resolver/resolver-abstract.ts:194 — // @ts-expect-error: overload are not correctly identified
- packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts:119 — // @ts-expect-error: to is never
- packages/router/src/experimental/route-resolver/old/resolver-dynamic.ts:121 — // @ts-expect-error: to is never
Please resolve these type mismatches (or replace @ts-expect-error with targeted fixes) before considering this experimental API stable.
There was a problem hiding this comment.
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.
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