fix(build-report): parse route exports with OXC AST#1260
fix(build-report): parse route exports with OXC AST#1260NathanDrake2406 wants to merge 3 commits into
Conversation
Route report classification relied on regexes and a hand-written return-object scanner. That made comments look like exports and missed valid static syntax such as no-substitution template literals and numeric separators. The report now parses route modules with oxc-parser, reads top-level static exports from the AST, and keeps getStaticProps revalidate extraction scoped to the exported function body. Tests cover the prior false positives, supported literal forms, TypeScript generic syntax fallback, and Next.js-style export specifier handling.
commit: |
The CLI imported the route report module at startup, which also loaded the native parser dependency added for build report classification. That widened the parser and native binding path to non-build commands. Load the build report module inside buildApp() immediately before printing the report so dev, start, check, lint, deploy, and help paths do not touch the parser-backed report code.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: AST-based route report classification
Strong improvement. Replacing hand-rolled token scanning with OXC parser output is the right direction — it eliminates the comment/string false-positive class of bugs entirely and handles TS syntax naturally. The code is well-structured, the test additions are targeted, and the fallback path for bare return-object snippets is a reasonable pragmatic choice.
A few items below, roughly ordered by importance.
Repeated parsing of the same source
This is the main structural concern. For a single Pages Router route, classifyPagesRoute can call parseRouteModule(code) 3–4 times on the same source string (once per hasNamedExport call, once in extractGetStaticPropsRevalidate, plus potentially the wrapped fallback). For App Router routes, classifyAppRoute parses twice (extractExportConstString + extractExportConstNumber). OXC is fast, but this is still O(routes × parses_per_route) with a constant of 2–4 where 1 would suffice.
This doesn't need to block the PR, but consider a follow-up that parses once at the classifier level and threads the Program through the helpers. The exported helpers can keep their code: string signature for external callers, with internal overloads that accept a pre-parsed AST.
Missing explicit return in extractStaticNumberValue
The UnaryExpression branch (line 256–261) falls through to the final return null when the operator isn't - or +, but it also falls through when the recursive call succeeds with - or +. The current code happens to work because falling through to return null at line 263 is harmless for unhandled operators, but the successful - / + cases are missing an explicit return. See inline comment.
hasNamedExport semantic change is intentional and correct
Verified that checking specifier.local (the original binding name) matches Next.js's checkExports implementation, which uses specifier.orig.value. The new test at line 62 correctly encodes this. Good documentation in the JSDoc.
cli.ts lazy import
Moving printBuildReport from a static import to a dynamic await import() is a nice touch — avoids pulling in oxc-parser at CLI startup for commands that don't need it.
Tests
Good regression coverage for the new capabilities (block comments, template literals, numeric separators, TS generic arrow fallback, alias export semantics). The wrapped-code fallback path (extractWrappedGetStaticPropsRevalidate) could use a dedicated test that exercises it specifically — right now it's only tested indirectly through the bare return { ... } cases.
| if (unwrapped.type === "UnaryExpression") { | ||
| const argument = extractStaticNumberValue(unwrapped.argument); | ||
| if (typeof argument !== "number") return null; | ||
| if (unwrapped.operator === "-") return -argument; | ||
| if (unwrapped.operator === "+") return argument; | ||
| } |
There was a problem hiding this comment.
The UnaryExpression branch is missing an explicit return for the - and + cases. The code works by accident because the fall-through hits return null at line 263, but only the unhandled-operator case should reach that. The - and + branches need to return their computed value:
| if (unwrapped.type === "UnaryExpression") { | |
| const argument = extractStaticNumberValue(unwrapped.argument); | |
| if (typeof argument !== "number") return null; | |
| if (unwrapped.operator === "-") return -argument; | |
| if (unwrapped.operator === "+") return argument; | |
| } | |
| if (unwrapped.operator === "-") return -argument; | |
| if (unwrapped.operator === "+") return argument; | |
| return null; | |
| } |
Without this, extractStaticNumberValue for -42 returns null instead of -42 — which would break export const revalidate = -1 detection.
Actually wait — I re-read this and the if block at line 256 is not early-returning, it's just an if with no braces-in-the-if-sense. Let me re-check.
OK, I see it now — the if at line 256 enters the block (lines 257–261), and if operator is -, it returns -argument. If operator is +, it returns argument. If neither, it falls through to line 263's return null. So the behavior is actually correct for all operators.
But: if the operator is something other than - or + (e.g., ~, !, void, typeof, delete), the code falls through the if body without returning, then hits the final return null. This is correct behavior but would be clearer with an explicit return null at the end of the if block to show intent. Minor style nit.
| } | ||
|
|
||
| function parseRouteModule(code: string): Program | null { | ||
| return parseRouteModuleWithLang(code, "tsx") ?? parseRouteModuleWithLang(code, "ts"); |
There was a problem hiding this comment.
Every call to parseRouteModule tries TSX first, then falls back to TS. For .ts files with generic arrow syntax (like the test at build-report.test.ts:173), this means two parseSync calls per parseRouteModule invocation — and since each classifier calls parseRouteModule 2–4 times on the same code, you can end up with 4–8 parseSync calls for a single route file.
Not blocking, but worth noting for the follow-up: if you parse once at the classifier level, the TSX→TS fallback cost is also paid only once.
| const localName = moduleExportNameValue(specifier.local); | ||
| if (localName !== "getStaticProps") continue; | ||
| if (node.source !== null) return "external"; | ||
| localExportNames.add(localName); |
There was a problem hiding this comment.
The set localExportNames will only ever contain the string "getStaticProps" because of the guard at line 284. So localExportNames.size is at most 1, and iterating over it later is always a single-element loop. This is fine but the Set<string> is slightly misleading — a simple let localBindingName: string | null = null would be clearer and avoid the allocation.
Minor nit, not blocking.
| const paramsStart = code.indexOf("(", functionStart); | ||
| if (paramsStart === -1) return -1; | ||
| function extractWrappedGetStaticPropsRevalidate(code: string): number | false | null { | ||
| const program = parseRouteModule(`function __vinextGetStaticProps() {\n${code}\n}`); |
There was a problem hiding this comment.
Clever fallback: wrapping the code in a synthetic function to make bare return { ... } statements parseable as module source. This handles the existing test cases where extractGetStaticPropsRevalidate is called on a code fragment rather than a full module.
Worth a brief comment explaining why this exists — the tests call extractGetStaticPropsRevalidate with bare return statements that aren't valid module source, so this wrapper makes them parseable. Without the comment, a future reader might wonder when parseRouteModule would succeed but findExportedGetStaticProps would return null (answer: it can't happen for well-formed modules, this path is for code fragments in tests and edge cases).
| if (declarationHasBindingName(node.declaration, name)) return true; | ||
|
|
||
| for (const specifier of node.specifiers) { | ||
| if (moduleExportNameValue(specifier.local) === name) { |
There was a problem hiding this comment.
Good — using specifier.local here correctly matches Next.js's checkExports which uses specifier.orig.value. The JSDoc at line 125–126 clearly documents this.
One subtlety worth noting: this means hasNamedExport answers "does this module have a local binding with this name that's exported?" rather than "does this module export this name?". If hasNamedExport is ever used for middleware or proxy export detection, it would need the opposite check (specifier.exported), per Next.js's validateMiddlewareProxyExports. The current usage for getStaticProps/getServerSideProps is correct.
| } | ||
|
|
||
| return findMatchingBrace(code, bodyStart); | ||
| if (!fn.body || fn.body.type !== "BlockStatement") return null; |
There was a problem hiding this comment.
The condition !fn.body can't actually be true here — FunctionDeclaration, FunctionExpression, and ArrowFunctionExpression all have a body property in OXC's AST. The arrow-with-expression-body case is already handled at line 340. This guard is defensive and harmless, but strictly unreachable for the types in FunctionLike.
|
Review submitted. Here's a summary of the feedback: Main structural concern: Each route classification parses the same source code 2–4 times via independent Inline comments posted (6):
|
Route classification reparsed the same source for each static export query. With the TSX-to-TS fallback, a single route could invoke the parser several times before producing one report row. Thread the parsed OXC Program through internal route-report helpers so classifier paths parse each source once while preserving the public string-based helper signatures. Also clarifies the synthetic getStaticProps wrapper path and the unsupported unary-operator branch.
|
Addressed the valid review items in
The unary branch was not a functional bug after re-checking, but the explicit return is clearer. Validation after the patch:
|
|
/bigbonk review |
There was a problem hiding this comment.
Re-review after author's response
The author addressed the substantive feedback from the first review well:
-
Parse-once threading — Classifiers (
classifyPagesRoute,classifyAppRoute,classifyLayoutSegmentConfig) now parse once and thread theProgramthrough internal*InProgram/*FromProgramhelpers. The exported string-signature helpers remain for external callers. This eliminates the repeated parsing. -
localExportNamesset replaced — Now useslet hasLocalGetStaticPropsExport = false, which is clearer for the single-value case. -
Explicit
return nullin unary branch — Added at line 289 for non--/+operators. -
Synthetic wrapper comment —
extractWrappedGetStaticPropsRevalidatenow documents why it exists (lines 354-355).
The code is well-structured, the test additions are targeted, and the overall approach (OXC parser replacing hand-rolled token scanning) is the right direction. The extractStatementRevalidate walker correctly avoids descending into nested function declarations/expressions by not handling VariableDeclaration initializers or FunctionDeclaration bodies — the AST naturally scopes the revalidate search to the exported function's own statements.
Pre-existing observation (not blocking)
When parseRouteModule(code) returns null in classifyPagesRoute (report.ts:534), neither the getServerSideProps nor getStaticProps checks run, and the function falls through to { type: "static" } at line 553. This silently treats unparseable files as static. The old regex code had the same behavior (no match → static), so this is parity-preserving. But "unknown" would be a safer default for genuinely unparseable source. Worth a follow-up if you want to tighten this.
LGTM — good to merge.
| } | ||
|
|
||
| return findMatchingBrace(code, bodyStart); | ||
| if (!fn.body || fn.body.type !== "BlockStatement") return null; |
There was a problem hiding this comment.
The !fn.body guard is still here — the previous review noted it's unreachable for the types in FunctionLike. Harmless, and arguably defensive-in-depth if the OXC AST types ever broaden. Fine to keep.
| depth--; | ||
| if (depth === 0) return i; | ||
| } | ||
| return extractStaticNumberValue(property.value); |
There was a problem hiding this comment.
Confirmed: extractStaticNumberValue can return false (for revalidate: false in the object literal), and that propagates correctly through extractObjectRevalidate → extractFunctionRevalidate → extractGetStaticPropsRevalidateFromProgram. The typing is sound.
| const paramsEnd = findMatchingParen(code, paramsStart); | ||
| if (paramsEnd === -1) return -1; | ||
| function extractWrappedGetStaticPropsRevalidate(code: string): number | false | null { | ||
| // Exported helpers are also used by tests with bare `return { ... }` fragments, |
There was a problem hiding this comment.
Good comment. This clearly explains the synthetic wrapper purpose.
|
Review submitted successfully. Here's a summary of what I posted: Approved the PR with the following feedback:
|
|
Can we use the |
2 participants
Overview
oxc-parser, then inspect top-level exports andgetStaticPropsreturn objects from the AST.packages/vinext/src/build/report.ts,tests/build-report.test.tsFixes #605.
Why
Route report generation needs syntactic facts about a module: exported names, literal segment config values, and top-level
getStaticPropsreturn objects. Regexes cannot reliably distinguish code from comments or nested helper functions, and they miss valid syntax once TypeScript and newer JavaScript literals enter the source. Next.js handles this class of work through a static analysis pass over a parsed module, so vinext should do the same at the route-report boundary.hasNamedExportnow readsExportNamedDeclarationnodes from OXC's AST.export constdeclarations only.dynamicandrevalidateextraction now unwrap TS assertions and supports string template literals plus numeric separators.getStaticPropsrevalidatebelongs to the exported function body, not arbitrary source text.checkExports.What changed
export const dynamic = "force-dynamic"export const revalidate = 60_00060000..tssyntax.getStaticPropscontains nested helper returnsValidation
pnpm exec vp test run tests/build-report.test.ts tests/layout-classification.test.ts tests/route-classification-manifest.test.ts tests/nitro-route-rules.test.ts- 4 files, 140 tests passedpnpm exec vp check packages/vinext/src/build/report.ts tests/build-report.test.ts packages/vinext/package.json pnpm-workspace.yaml- formatting, lint, and type checks passed for touched source/test filespnpm exec vp fmt --check packages/vinext/src/build/report.ts tests/build-report.test.ts packages/vinext/package.json pnpm-workspace.yaml- formatting passedpnpm exec vp run vinext#build- package build completedpnpm install --frozen-lockfile --lockfile-only- lockfile consistency passedgit diff --check- no whitespace errorsRisk / compatibility
oxc-parserdependency forpackages/vinext.oxc-parser@0.131.0as latest, but it was published on 2026-05-15 at 16:03 UTC. The repo hasminimumReleaseAge: 1440, and verification was at 2026-05-16 07:53 UTC, so this PR uses0.130.0, the newest version that clears the repo's 24-hour package-age policy.getStaticPropsremain intentionally unresolved, matching the previous report boundary and Next's static-analysis style.Maintainer review path
packages/vinext/src/build/report.ts- review the OXC parser boundary, static export helpers, andgetStaticPropsAST walking.tests/build-report.test.ts- review the regression cases for comment false positives, literal syntax support, TS parser fallback, and Next-style export specifier handling.packages/vinext/package.json,pnpm-workspace.yaml,pnpm-lock.yaml- review the direct parser dependency and locked native bindings.References
get-page-static-info.tsparse-module.tsextract-const-value.ts