From b894ef7ebdf70daa1b929b8afbf0840fcf5bd3e0 Mon Sep 17 00:00:00 2001 From: skirtle <65301168+skirtles-code@users.noreply.github.com> Date: Sat, 10 Feb 2024 14:47:24 +0000 Subject: [PATCH 1/3] perf: use a binary search for insertMatcher --- packages/router/src/matcher/index.ts | 65 ++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 18 deletions(-) diff --git a/packages/router/src/matcher/index.ts b/packages/router/src/matcher/index.ts index 39a3c24b1..9ad6f2bc1 100644 --- a/packages/router/src/matcher/index.ts +++ b/packages/router/src/matcher/index.ts @@ -223,17 +223,8 @@ export function createRouterMatcher( } function insertMatcher(matcher: RouteRecordMatcher) { - let i = 0 - while ( - i < matchers.length && - comparePathParserScore(matcher, matchers[i]) >= 0 && - // Adding children with empty path should still appear before the parent - // https://github.com/vuejs/router/issues/1124 - (matcher.record.path !== matchers[i].record.path || - !isRecordChildOf(matcher, matchers[i])) - ) - i++ - matchers.splice(i, 0, matcher) + const index = findInsertionIndex(matcher, matchers) + matchers.splice(index, 0, matcher) // only add the original record to the name map if (matcher.record.name && !isAliasRecord(matcher)) matcherMap.set(matcher.record.name, matcher) @@ -520,13 +511,51 @@ function checkMissingParamsInAbsolutePath( } } -function isRecordChildOf( - record: RouteRecordMatcher, - parent: RouteRecordMatcher -): boolean { - return parent.children.some( - child => child === record || isRecordChildOf(record, child) - ) +/** + * Performs a binary search to find the correct insertion index for a new matcher. + * + * Matchers are primarily sorted by their score. If scores are tied then the matcher's depth is used instead. + * The depth check ensures that a child with an empty path comes before its parent. + * + * @param matcher - new matcher to be inserted + * @param matchers - existing matchers + */ +function findInsertionIndex( + matcher: RouteRecordMatcher, + matchers: RouteRecordMatcher[] +) { + const depth = getDepth(matcher) + + let lower = 0 + let upper = matchers.length + + while (lower !== upper) { + const mid = (lower + upper) >> 1 + const sortOrder = + comparePathParserScore(matcher, matchers[mid]) || + getDepth(matchers[mid]) - depth + + if (sortOrder === 0) { + return mid + } else if (sortOrder < 0) { + upper = mid + } else { + lower = mid + 1 + } + } + + return upper +} + +function getDepth(record: RouteRecordMatcher) { + let depth = 0 + + while (record.parent) { + ++depth + record = record.parent + } + + return depth } export type { PathParserOptions, _PathParserOptions } From 48db602f57375cca3f3280c90d79853b4aa397c3 Mon Sep 17 00:00:00 2001 From: skirtle <65301168+skirtles-code@users.noreply.github.com> Date: Sat, 24 Feb 2024 02:12:27 +0000 Subject: [PATCH 2/3] Fix matcher ordering in edge cases --- packages/router/src/matcher/index.ts | 70 +++++++++++++++++----------- 1 file changed, 43 insertions(+), 27 deletions(-) diff --git a/packages/router/src/matcher/index.ts b/packages/router/src/matcher/index.ts index 9ad6f2bc1..40f0d58db 100644 --- a/packages/router/src/matcher/index.ts +++ b/packages/router/src/matcher/index.ts @@ -158,6 +158,12 @@ export function createRouterMatcher( removeRoute(record.name) } + // Avoid adding a record that doesn't display anything. This allows passing through records without a component to + // not be reached and pass through the catch all route + if (isMatchable(matcher)) { + insertMatcher(matcher) + } + if (mainNormalizedRecord.children) { const children = mainNormalizedRecord.children for (let i = 0; i < children.length; i++) { @@ -177,17 +183,6 @@ export function createRouterMatcher( // if (parent && isAliasRecord(originalRecord)) { // parent.children.push(originalRecord) // } - - // Avoid adding a record that doesn't display anything. This allows passing through records without a component to - // not be reached and pass through the catch all route - if ( - (matcher.record.components && - Object.keys(matcher.record.components).length) || - matcher.record.name || - matcher.record.redirect - ) { - insertMatcher(matcher) - } } return originalMatcher @@ -514,8 +509,8 @@ function checkMissingParamsInAbsolutePath( /** * Performs a binary search to find the correct insertion index for a new matcher. * - * Matchers are primarily sorted by their score. If scores are tied then the matcher's depth is used instead. - * The depth check ensures that a child with an empty path comes before its parent. + * Matchers are primarily sorted by their score. If scores are tied then we also consider parent/child relationships, + * with descendants coming before ancestors. If there's still a tie, new routes are inserted after existing routes. * * @param matcher - new matcher to be inserted * @param matchers - existing matchers @@ -524,38 +519,59 @@ function findInsertionIndex( matcher: RouteRecordMatcher, matchers: RouteRecordMatcher[] ) { - const depth = getDepth(matcher) - + // First phase: binary search based on score let lower = 0 let upper = matchers.length while (lower !== upper) { const mid = (lower + upper) >> 1 - const sortOrder = - comparePathParserScore(matcher, matchers[mid]) || - getDepth(matchers[mid]) - depth + const sortOrder = comparePathParserScore(matcher, matchers[mid]) - if (sortOrder === 0) { - return mid - } else if (sortOrder < 0) { + if (sortOrder < 0) { upper = mid } else { lower = mid + 1 } } + // Second phase: check for an ancestor with the same score + const insertionAncestor = getInsertionAncestor(matcher) + + if (insertionAncestor) { + upper = matchers.lastIndexOf(insertionAncestor, upper - 1) + + if (__DEV__ && upper === -1) { + // This should never happen + warn( + `Finding ancestor route ${insertionAncestor.record.path} failed for ${matcher.record.path}` + ) + } + } + return upper } -function getDepth(record: RouteRecordMatcher) { - let depth = 0 +function getInsertionAncestor(matcher: RouteRecordMatcher) { + let ancestor: RouteRecordMatcher | undefined = matcher - while (record.parent) { - ++depth - record = record.parent + while ((ancestor = ancestor.parent)) { + if ( + isMatchable(ancestor) && + comparePathParserScore(matcher, ancestor) === 0 + ) { + return ancestor + } } - return depth + return +} + +function isMatchable({ record }: RouteRecordMatcher): boolean { + return !!( + record.name || + (record.components && Object.keys(record.components).length) || + record.redirect + ) } export type { PathParserOptions, _PathParserOptions } From 87320b738b21f6289f5ac79e4e0762ba32c8584d Mon Sep 17 00:00:00 2001 From: Eduardo San Martin Morote Date: Mon, 10 Jun 2024 15:16:51 +0200 Subject: [PATCH 3/3] chore: docs and logs --- packages/router/src/matcher/index.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/router/src/matcher/index.ts b/packages/router/src/matcher/index.ts index 0d3b2d854..7040fa810 100644 --- a/packages/router/src/matcher/index.ts +++ b/packages/router/src/matcher/index.ts @@ -545,10 +545,10 @@ function findInsertionIndex( if (insertionAncestor) { upper = matchers.lastIndexOf(insertionAncestor, upper - 1) - if (__DEV__ && upper === -1) { + if (__DEV__ && upper < 0) { // This should never happen warn( - `Finding ancestor route ${insertionAncestor.record.path} failed for ${matcher.record.path}` + `Finding ancestor route "${insertionAncestor.record.path}" failed for "${matcher.record.path}"` ) } } @@ -571,6 +571,13 @@ function getInsertionAncestor(matcher: RouteRecordMatcher) { return } +/** + * Checks if a matcher can be reachable. This means if it's possible to reach it as a route. For example, routes without + * a component, or name, or redirect, are just used to group other routes. + * @param matcher + * @param matcher.record record of the matcher + * @returns + */ function isMatchable({ record }: RouteRecordMatcher): boolean { return !!( record.name ||