feat: precompile next.config matchers at build time#536
feat: precompile next.config matchers at build time#536SeolJaeHyeok wants to merge 6 commits intocloudflare:mainfrom
Conversation
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
Review: Precompile config matchers at build time
Good idea — moving regex compilation out of the request path is a clear cold-start win. The refactoring of compileConfigPattern and compileHeaderSourcePattern into reusable helpers is clean, and the threading through all four server paths (dev, App Router entry, Pages Router entry, deploy entry) is thorough.
However, there's one significant correctness issue: the precompiled matchRedirect path regresses to O(n) linear scanning, bypassing the locale-static index optimization that was specifically added to handle apps with 63+ locale-prefixed redirect rules. There are also a couple of smaller issues worth addressing.
Summary of issues
-
[Blocking]
matchRedirectprecompiled path bypasses the locale-static index — theif (compiledPatterns)early-return path does a linear scan over all redirects, losing the O(1) map-lookup optimization for locale-prefixed rules. For apps with many locale redirects, this reintroduces the exact O(n × regex) cost that the index was built to eliminate. -
[Non-blocking] No serialization roundtrip test — the tests verify that generated code contains
__compiledRedirectsand regex-looking strings, but no test evaluates the serialized code to confirm the regex survives theRegExp.toString()→ code-embed →evalcycle. A single roundtrip test would catch subtle serialization bugs (e.g., regex flags, special characters in param names). -
[Non-blocking]
serializeCompiledPatterndoesn't escapeparamNames—JSON.stringifyhandles this correctly for normal param names, but it's worth noting that if a param name ever contained characters that are special in JSON strings (unlikely but possible with future pattern extensions), the current approach is safe because ofJSON.stringify. -
[Non-blocking] Comment stale in
matchHeaders— the comment on line 1107 still referencesescapeHeaderSource() + safeRegExp()as the cache rationale, but the code now has a two-tier lookup (precompiled array first, then module-level cache). The comment should be updated to reflect the new lookup order.
| if (compiledPatterns) { | ||
| for (let i = 0; i < redirects.length; i++) { | ||
| const redirect = redirects[i]; | ||
| const compiled = compiledPatterns[i]; | ||
| const params = compiled | ||
| ? execCompiledConfigPattern(pathname, compiled) | ||
| : matchConfigPattern(pathname, redirect.source); | ||
| if (!params) continue; | ||
| if (redirect.has || redirect.missing) { | ||
| if (!checkHasConditions(redirect.has, redirect.missing, ctx)) { | ||
| continue; | ||
| } | ||
| } | ||
| let dest = substituteDestinationParams(redirect.destination, params); | ||
| dest = sanitizeDestination(dest); | ||
| return { destination: dest, permanent: redirect.permanent }; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Blocking: This precompiled fast path does a linear O(n) scan over all redirects, which bypasses the locale-static index optimization (_getRedirectIndex) that exists specifically for apps with many locale-prefixed redirect rules (63+ rules in the motivating profile).
When compiledPatterns is passed, the locale-static O(1) map lookup + tiny-regex validation is never used — every request linearly tests every redirect pattern. For simple apps this is fine, but for the apps that motivated the locale-static index, this is a regression.
Suggested approach: instead of a separate early-return branch, integrate the precompiled patterns into the existing matchRedirect flow. The locale-static index handles locale patterns; for the linear fallback rules, you could pass the precompiled pattern by original index so matchConfigPattern is skipped when a precompiled version exists. Something like:
// In the linear fallback loop:
for (const [origIdx, redirect] of index.linear) {
if (origIdx >= localeMatchIndex) break;
const compiled = compiledPatterns?.[origIdx];
const params = compiled
? execCompiledConfigPattern(pathname, compiled)
: matchConfigPattern(pathname, redirect.source);
// ...
}This preserves the locale-static fast path while still benefiting from precompilation for non-locale rules.
| // Cache the compiled source regex — escapeHeaderSource() + safeRegExp() are | ||
| // pure functions of rule.source and the result never changes between requests. | ||
| let sourceRegex = _compiledHeaderSourceCache.get(rule.source); | ||
| let sourceRegex = compiledSources?.[i]; |
There was a problem hiding this comment.
Subtle but correct: when compiledSources[i] is null (safeRegExp rejected the pattern at build time), null !== undefined so we correctly skip the runtime cache lookup and fall through to the if (sourceRegex && ...) check which is falsy for null. Good.
However, the comment above (line 1107-1108) is now stale — it describes the old single-tier cache but the code now has a two-tier lookup (precompiled array → module-level cache). Consider updating the comment to reflect the new flow.
|
|
||
| function serializeCompiledPattern(pattern: CompiledConfigPattern | null): string { | ||
| if (!pattern) return "null"; | ||
| return `{ re: ${pattern.re.toString()}, paramNames: ${JSON.stringify(pattern.paramNames)} }`; |
There was a problem hiding this comment.
Minor concern: RegExp.prototype.toString() produces /pattern/flags, which is valid JavaScript when embedded directly in generated code. However, if the regex pattern itself contains a forward slash (e.g., from an escaped path separator), toString() should still produce correct output since the slash is escaped in the regex source. This works correctly for all patterns I can see in the codebase.
That said, it would be good to have a roundtrip test that:
- Calls
buildPrecompiledConfigCodewith a non-trivial config - Evaluates the generated code string (e.g., via
new Function) - Verifies the deserialized regex matches the same inputs as the original
This would catch any edge cases in the serialization format.
| } | ||
|
|
||
| export function compileConfigPattern(pattern: string): CompiledConfigPattern | null { | ||
| if (!usesRegexBranch(pattern)) return null; |
There was a problem hiding this comment.
Worth noting: compileConfigPattern returns null for "simple" patterns (no regex groups, no backslashes, no catch-all suffixes, no dot after param). This means the precompiled arrays will contain null for these patterns, and the matching functions correctly fall back to matchConfigPattern at runtime.
This is a sensible design — simple segment-based patterns are already fast (string split + compare), so there's no need to precompile them into regexes. The doc comment on the exported function should mention this return-null behavior since callers need to understand that null means "use the runtime matcher" rather than "pattern is invalid."
| @@ -1472,7 +1477,7 @@ export default async function handler(request, ctx) { | |||
| let pathname; | |||
There was a problem hiding this comment.
Nit: this line is quite long (exceeds typical line width). Consider breaking it for readability, matching the style used for the afterFiles and fallback calls below:
| let pathname; | |
| const __rewritten = matchRewrite( | |
| cleanPathname, | |
| __configRewrites.beforeFiles, | |
| __postMwReqCtx, | |
| __compiledRewrites.beforeFiles, | |
| ); |
|
Review submitted with "Changes Requested". Here's a summary of the feedback: Blocking issue: The precompiled Non-blocking items:
|
[Blocking] matchRedirect with compiledPatterns bypassed the locale-static index entirely — the if (compiledPatterns) early-return ran a plain O(n×regex) linear scan, reintroducing the ~2992ms self-time the index was built to eliminate on apps with 63+ locale-prefixed rules. Remove the independent early-return block and absorb compiledPatterns into the existing linear fallback loop via compiledPatterns?.[origIdx], so the locale-static O(1) map-lookup runs unconditionally regardless of whether compiledPatterns is supplied. [Non-blocking] Add tests/precompiled-config.test.ts with serialization roundtrip tests that evaluate the output of buildPrecompiledConfigCode via new Function() and assert the resulting RegExp objects match and reject paths correctly. Covers inline regex groups, named params with alternation constraints, hyphenated param names, and simple patterns that compile to null. [Non-blocking] serializeCompiledPattern uses JSON.stringify for paramNames, which correctly handles any special characters — no code change needed. [Non-blocking] Update the matchHeaders comment to reflect the two-tier source lookup order: precompiled array first, module-level cache as fallback.
|
/bigbonk re-review |
There was a problem hiding this comment.
Review
Good work. The core idea — moving regex compilation out of the request path — is sound and well-executed. The refactoring into compileConfigPattern / compileHeaderSourcePattern helpers is clean, the threading through all four server paths is thorough, and the serialization roundtrip tests in precompiled-config.test.ts are exactly the right level of coverage.
Correction on the previous review's blocking issue
The previous automated review flagged that matchRedirect bypasses the locale-static index. That is incorrect. Looking at the actual code in config-matchers.ts:770-875, the locale-static index (_getRedirectIndex) is still built and the O(1) map lookup (lines 796-843) is fully preserved. The compiledPatterns array is only used inside the index.linear fallback loop at line 856, indexed by origIdx — which is exactly the approach the reviewer suggested as a fix. The code already does what was requested. This is not blocking.
Actual issues
-
[Nit] Formatting inconsistency — the
beforeFilesmatchRewritecall inapp-rsc-entry.ts:1677is a single long line, while theafterFilesandfallbackcalls below it are properly broken across multiple lines. Should be consistent. -
[Nit] Missing JSDoc on
compileConfigPattern— the exported function returnsnullfor simple patterns that don't need regex compilation (no groups, no backslashes, no catch-all suffixes, no dot-after-param). Callers need to understand thatnullmeans "use the runtime segment matcher" rather than "pattern is invalid." A brief doc comment would prevent future confusion.
Overall this looks ready to merge after the formatting nit.
| // has/missing conditions can evaluate against middleware-modified headers. | ||
| if (__configRewrites.beforeFiles && __configRewrites.beforeFiles.length) { | ||
| const __rewritten = matchRewrite(cleanPathname, __configRewrites.beforeFiles, __postMwReqCtx); | ||
| const __rewritten = matchRewrite(cleanPathname, __configRewrites.beforeFiles, __postMwReqCtx, __compiledRewrites.beforeFiles); |
There was a problem hiding this comment.
Nit: this line is quite long compared to the afterFiles and fallback calls below it, which are properly broken across multiple lines. Should be consistent:
| const __rewritten = matchRewrite(cleanPathname, __configRewrites.beforeFiles, __postMwReqCtx, __compiledRewrites.beforeFiles); | |
| const __rewritten = matchRewrite( | |
| cleanPathname, | |
| __configRewrites.beforeFiles, | |
| __postMwReqCtx, | |
| __compiledRewrites.beforeFiles, | |
| ); |
|
Review submitted. Here's a summary of what I found: The previous automated review's blocking issue was incorrect. The Actual findings (both minor nits):
The serialization roundtrip tests in |
Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com>
3a761b4 to
d057788
Compare
Closes #389
summary
compileConfigPatternandcompileHeaderSourcePatternhelpers fromconfig-matcherswhat changed
packages/vinext/src/config/precompiled-config.tsto build and serialize precompiled config matcher statepackages/vinext/src/config/config-matchers.tsto expose reusable compile helpers and optional compiled matcher inputs for redirects, rewrites,and headers
generateRscEntry()to emit__compiledRedirects,__compiledRewrites, and__compiledHeadersalongside the existing config payloadsgenerateServerEntry()to exportvinextCompiledConfigfor Pages Router production and deploy pathsindex.ts,server/prod-server.ts, anddeploy.tsto thread the precompiled matcher state through the existing config matching flowwhy
PR #387 removed the steady-state bottleneck by caching compiled config patterns per isolate. That keeps warm isolates fast, but low-traffic routes can still
pay the compilation cost on the first request after a cold start.
This PR moves that work out of the request path by compiling config patterns ahead of time for generated entries, and once during dev startup for the live
config path. Matching behavior stays the same, but the first matching request no longer needs to tokenize patterns, run the safe-regex check, and construct
RegExpinstances.test plan
pnpm test tests/shims.test.ts tests/app-router.test.ts tests/entry-templates.test.tspnpm run typecheckpnpm run lint