-
Notifications
You must be signed in to change notification settings - Fork 218
perf: replace O(n) linear route matching with radix trie #442
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
1c9f2c5
b1de57a
f284871
fc2823d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -24,6 +24,10 @@ const __dirname = path.dirname(fileURLToPath(import.meta.url)); | |||||
| const _requestContextShimPath = fileURLToPath( | ||||||
| new URL("../shims/request-context.js", import.meta.url), | ||||||
| ).replace(/\\/g, "/"); | ||||||
| const _routeTriePath = fileURLToPath(new URL("../routing/route-trie.js", import.meta.url)).replace( | ||||||
| /\\/g, | ||||||
| "/", | ||||||
| ); | ||||||
|
|
||||||
| /** | ||||||
| * Generate the virtual SSR server entry module. | ||||||
|
|
@@ -263,6 +267,7 @@ import { getSSRFontLinks as _getSSRFontLinks, getSSRFontStyles as _getSSRFontSty | |||||
| import { getSSRFontStyles as _getSSRFontStylesLocal, getSSRFontPreloads as _getSSRFontPreloadsLocal } from "next/font/local"; | ||||||
| import { parseCookies } from ${JSON.stringify(path.resolve(__dirname, "../config/config-matchers.js").replace(/\\/g, "/"))}; | ||||||
| import { runWithExecutionContext as _runWithExecutionContext, getRequestExecutionContext as _getRequestExecutionContext } from ${JSON.stringify(_requestContextShimPath)}; | ||||||
| import { buildRouteTrie as _buildRouteTrie, trieMatch as _trieMatch } from ${JSON.stringify(_routeTriePath)}; | ||||||
| ${instrumentationImportCode} | ||||||
| ${middlewareImportCode} | ||||||
|
|
||||||
|
|
@@ -339,51 +344,21 @@ ${docImportCode} | |||||
| const pageRoutes = [ | ||||||
| ${pageRouteEntries.join(",\n")} | ||||||
| ]; | ||||||
| const _pageRouteTrie = _buildRouteTrie(pageRoutes); | ||||||
|
|
||||||
| const apiRoutes = [ | ||||||
| ${apiRouteEntries.join(",\n")} | ||||||
| ]; | ||||||
| const _apiRouteTrie = _buildRouteTrie(apiRoutes); | ||||||
|
|
||||||
| function matchRoute(url, routes) { | ||||||
| const pathname = url.split("?")[0]; | ||||||
| let normalizedUrl = pathname === "/" ? "/" : pathname.replace(/\\/$/, ""); | ||||||
| // NOTE: Do NOT decodeURIComponent here. The pathname is already decoded at | ||||||
| // the entry point. Decoding again would create a double-decode vector. | ||||||
| const urlParts = normalizedUrl.split("/").filter(Boolean); | ||||||
| for (const route of routes) { | ||||||
| const params = matchPattern(urlParts, route.patternParts); | ||||||
| if (params !== null) return { route, params }; | ||||||
| } | ||||||
| return null; | ||||||
| } | ||||||
|
|
||||||
| function matchPattern(urlParts, patternParts) { | ||||||
| const params = Object.create(null); | ||||||
| for (let i = 0; i < patternParts.length; i++) { | ||||||
| const pp = patternParts[i]; | ||||||
| if (pp.endsWith("+")) { | ||||||
| if (i !== patternParts.length - 1) return null; | ||||||
| const paramName = pp.slice(1, -1); | ||||||
| const remaining = urlParts.slice(i); | ||||||
| if (remaining.length === 0) return null; | ||||||
| params[paramName] = remaining; | ||||||
| return params; | ||||||
| } | ||||||
| if (pp.endsWith("*")) { | ||||||
| if (i !== patternParts.length - 1) return null; | ||||||
| const paramName = pp.slice(1, -1); | ||||||
| params[paramName] = urlParts.slice(i); | ||||||
| return params; | ||||||
| } | ||||||
| if (pp.startsWith(":")) { | ||||||
| if (i >= urlParts.length) return null; | ||||||
| params[pp.slice(1)] = urlParts[i]; | ||||||
| continue; | ||||||
| } | ||||||
| if (i >= urlParts.length || urlParts[i] !== pp) return null; | ||||||
| } | ||||||
| if (urlParts.length !== patternParts.length) return null; | ||||||
| return params; | ||||||
| const trie = routes === pageRoutes ? _pageRouteTrie : _apiRouteTrie; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This
Suggested change
Actually, this is what you already have — but the fallback case is a silent wrong-answer rather than a loud error. A defensive check would be safer: const trie = routes === pageRoutes ? _pageRouteTrie
: routes === apiRoutes ? _apiRouteTrie
: (() => { throw new Error('matchRoute called with unknown routes array'); })();This is a minor robustness improvement for generated code. If a third route array is ever added, the current code would silently match against the API trie. |
||||||
| return _trieMatch(trie, urlParts); | ||||||
| } | ||||||
|
|
||||||
| function parseQuery(url) { | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot flagged this and I agree:
matchRoute(url)no longer takes aroutesparameter but the function is called from_handleRequestwhich previously passedroutesexplicitly. The unused parameter was removed, which is good.However, the function now implicitly closes over
_routeTrie(a module-level constant). This is fine for the current use case where there's only one route set, but it means the function signature no longer communicates what it matches against. A one-line JSDoc comment would help future readers: