fix(build): align Pages data export transforms - #2055
Conversation
commit: |
|
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
/bigbonk review — fix(build): align Pages data export transforms
Reviewed the transform port, the two plugin wirings in index.ts, and the new tests against the Next.js source in .nextjs-ref. The core strip-server-exports.ts transform is well-tested and the _app|_document|_error anchoring fix is a genuine parity improvement. Two issues worth addressing before merge — one is a deliberate-looking divergence from Next.js, the other is a perf regression in a hot path.
1. export * is rejected in API routes — Next.js does not do this (parity divergence)
packages/vinext/src/index.ts:4011-4032 — the new vinext:validate-page-exports plugin runs validatePageExports() on every page file under pages/, in every environment, with no API-route exclusion. The PR description and the code comment both state that "Next.js rejects export * from "..." in every Pages Router page file, including API routes." That is not what the Next.js source does.
In Next.js the disallow plugin is gated on !isServer && isPageFile:
// .nextjs-ref/packages/next/src/build/babel/loader/get-config.ts:192-200
const disallowExportAllItem =
!isServer && isPageFile
? createConfigItem([require('../plugins/next-page-disallow-re-export-all-exports')], ...)
: nullAPI routes never get a client compilation — createEntrypoints only routes them to onServer()/onEdgeServer():
// .nextjs-ref/packages/next/src/build/entries.ts:348-355
if (isAPIRoute(params.page)) {
if (isEdgeRuntime(params.pageRuntime)) { params.onEdgeServer(); return }
params.onServer(); return
}So export * from "..." in pages/api/* builds fine in Next.js. The new test tests/pages-server-exports-security.test.ts:175 ("rejects export-all declarations in API page files") asserts the opposite of Next.js behavior and the upstream test (test/production/re-export-all-exports-from-page-disallowed/) only covers regular pages (about.js, contact.js, index.js) — no API route case.
This will break valid Next.js apps that use export * in an API route. Either:
- exclude API routes (and
_app/_document) fromvalidate-page-exportsto match Next.js, or - if this is an intentional defense-in-depth divergence, document it as a deliberate choice (per AGENTS.md, divergences must be deliberate and documented) and fix the comment + PR description, which currently misstate Next.js behavior.
2. Both transform handlers now realpathSync every module on every transform (perf regression)
packages/vinext/src/index.ts:4015-4032 and :4041-4067. The previous strip-server-exports plugin had filter: { id: /\.(tsx?|jsx?|mjs)$/ } and used the cheap id.startsWith(pagesDir) guard first. Both were removed:
vinext:validate-page-exportshas nofilterand no environment check, so its handler runs for every module across the client, SSR, and RSC environments.vinext:strip-server-exportslost its extensionfilter.
Both now call canonicalize(stripViteModuleQuery(id)) unconditionally, and canonicalize is fs.realpathSync.native(...) (uncached syscall — build/ssr-manifest.ts:16). That is an extra syscall per module per environment, including for \0-prefixed virtual modules, CSS, and node_modules, none of which can be pages.
Suggested mitigations (cheap, no behavior change):
- Re-add a
filter: { id: /\.(tsx?|jsx?|mjs|mdx)$/ }to both plugins so the handler is skipped entirely for non-source modules. - Add an early
if (id.startsWith("\0")) return null;beforecanonicalize. - For
validate-page-exports, short-circuit on the cheap!/\bexport\s*\*/.test(code)check before doing the path work (this is the same check already insidevalidatePageExports).
Things that look correct
- Anchoring
_app/_document/_errorto^/(?:_app|_document|_error)(?:\.[^/]*)?$correctly stops treating nestedpages/admin/_error.tsxas special — matches Next.js, and the security fixture confirms its server deps are stripped. - The transform's dependency sweep, lexical-shadow handling, partial destructuring pruning, and mixed SSR/SSG conflict error all have solid, Next.js-aligned test coverage.
vp test run tests/build-optimization.test.tspasses (141/141) locally. validatePageExportscorrectly uses a broader\bexport\s*\*regex than thecode.includes("export *")fast-path instripServerExports, so newline-separatedexport\n*\nfromis still caught by the dedicated plugin.
One minor follow-up to consider: recursive arrow-function helpers used only by server exports won't be swept, because the self-reference exclusion at strip-server-exports.ts:566-569 only applies to function/class kinds, not variable. Not a regression, just a parity gap if you want full coverage.
|
Review posted to PR #2055. Summary of findings: Two issues to address1. 2. Perf regression — What's correct
One minor note: recursive arrow-function helpers used only by server exports won't be swept (the self-reference exclusion only covers |
|
Addressed the review findings in d616c14:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — fix(build): align Pages data export transforms (d616c14)
Re-reviewed after the two findings from the prior review were addressed. Both are now resolved, and I verified the fixes against the Next.js source in .nextjs-ref and by running the affected suites locally (vp test run → 149 passed across build-optimization + pages-server-exports-security, plus middleware-server-only 4 passed; vp check clean on both source files).
Finding 1 (API-route export * parity) — resolved ✅
validate-page-exports now early-returns for API routes via isApiPage(canonicalId) (index.ts:4053), matching Next.js, where the disallow-export-all plugin is gated on !isServer && isPageFile and API routes never get a client compilation. The test was flipped from "rejects" to "allows export-all declarations in API page file" and is parameterized over both pages/api.ts and pages/api/index.ts (pages-server-exports-security.test.ts:181), covering the root-API-file edge case too. The misleading comment/description were corrected.
Finding 2 (per-module realpathSync perf regression) — resolved ✅
Both handlers now gate canonicalization behind cheap checks before any syscall:
\0virtual-module early-return (index.ts:4042,:4072).validate-page-exports: cheapcode.includes("export") && code.includes("*")short-circuit, thenisWithinPagesDirectory(modulePath)(string prefix) beforecanonicalize.strip-server-exports:hasServerExportCandidate(code)fast-path, then the same string-prefix guard beforecanonicalize.- Canonical paths are memoized in
pageTransformCanonicalPaths(bounded by page-module count, since only pages-dir candidates reach it).
Dropping the /\.(tsx?|jsx?|mjs)$/ filter in favor of fileMatcher.isPageFile is actually a correctness improvement: the old filter excluded configured pageExtensions like mdx, and the MDX fixture in the security test now exercises that path.
Recursive-helper parity gap — resolved ✅
The dependency sweep was reworked into a fixpoint (removableBindings seed → closureChanged growth over implementation ranges → pruneChanged shrink on live references). This is implementation-based rather than function/class-kind-based, so recursive and mutually-recursive arrow helpers used only by data exports are now swept (new tests at build-optimization.test.ts:2873, :2889), while client-shared bindings are still preserved (verified by the aliased/shared-binding tests).
Spot-checks that look correct
_app|_document|_erroranchoring (^/(?:_app|_document|_error)(?:\.[^/]*)?$) — nestedpages/admin/_error.tsxis treated as a regular page; the security fixture confirms its server deps + secret are stripped from client output._app/_documentare not excluded fromvalidate-page-exports(only API routes are), matching Next.jsisPageFilegating.- Plugin reorder (server-only validation now runs after the data-export strip) is sound: the strip removes the import edge in the page module, so a server-only module re-exported only via
getServerSidePropsnever enters the client graph. Confirmed by theindex.tsxre-export fixture building cleanly.
Minor (non-blocking)
validatePageExports'scode.includes("*")guard will parse files that contain a multiplication but no export-all; harmless, just an occasional extra parse.pageTransformCanonicalPathshas no eviction, but it's bounded by the number of page candidates, so not a concern.
LGTM. No blocking issues.
|
Re-review posted to PR #2055. Summary: Verdict: LGTM — no blocking issuesThe two findings from the prior review (commit 6918345) are both resolved in d616c14, verified against Finding 1 — API-route Finding 2 — per-module Bonus — recursive-helper parity gap (resolved): The dependency sweep was reworked into an implementation-based fixpoint, so recursive and mutually-recursive arrow helpers used only by data exports are now swept while shared client bindings are preserved. New tests cover both. Verification: 149 tests passed (build-optimization + security), middleware-server-only 4 passed, Only two non-blocking minor notes (harmless extra parse from the |
|
Addressed the remaining export-all prefilter note: the plugin hook, validator, and strip transform now share a comment-aware |
|
/bigbonk review for issues |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
/bigbonk re-review — fix(build): align Pages data export transforms (536a42c)
Re-reviewed after the export-all prefilter rework in commit 536a42c. The change is sound — no blocking issues. Verified by running the affected suites locally (vp test run tests/build-optimization.test.ts tests/pages-server-exports-security.test.ts → 150 passed; vp check clean on both source files).
New since last LGTM: shared hasExportAllCandidate() guard — looks correct ✅
The three call sites (validate-page-exports plugin hook in index.ts:4041, validatePageExports at strip-server-exports.ts:294, and stripServerExports at :317) now share one comment-aware regex:
/\bexport(?:\s|\/\*[\s\S]*?\*\/|\/\/[^\r\n]*(?:\r?\n|$))*\*/I tested this directly against a range of inputs:
- Real export-all forms all match:
export * from,export\n*\nfrom,export /* c */ * from,export // c\n* from, tabs, multiple consecutive comments, CRLF, andexport * as ns from. No realexport *form is missed. - Multiplication no longer false-positives:
export const area = width * height;andexport {a} from "x"; const y = 2 * 3;correctly returnfalse, which was the whole point of this commit (the oldcode.includes("export") && code.includes("*")guard would parse these). export default a * bcorrectly returnsfalse.- No ReDoS: pathological inputs (100k-char unterminated comment, 500k newlines without a star) complete in <10ms — the alternation is anchored on
exportand each branch consumes distinct character classes, so there's no catastrophic backtracking.
The remaining false positives (export * appearing only inside a string literal or comment) are harmless: they just trigger one extra parseAst, and validatePageExports only throws on an actual ExportAllDeclaration node (:301-305), matching Next.js's next-page-disallow-re-export-all-exports AST-visitor behavior. This is the same harmless-extra-parse note from the prior review, now narrowed further.
Spot-checks that remain correct
stripServerExportsguard parity: the plugin only invokes it whenhasServerExportCandidate(code)is true (index.ts:4068), so the|| !hasExportAllCandidate(code)branch insidestripServerExports(:317) is effectively dead in the plugin path — but it's still exercised by direct unit tests, andexport *rejection is owned by the separatevalidate-page-exportsplugin, so there's no behavioral gap.- API-route /
_app/_document/_errorgating unchanged and still correct:isApiPagecovers rootpages/api.<ext>(stripExtension("/api") === "/api") and nested/api/*; the_app|_document|_erroranchor (^/(?:_app|_document|_error)(?:\.[^/]*)?$) keeps nestedpages/admin/_error.tsxas a regular page. - Canonical-path cache is still keyed after the
isWithinPagesDirectory(modulePath)guard, so it stays bounded by page-candidate count. - New test (
build-optimization.test.ts:2429) covers the multiplication-vs-export-all distinction and the comment/whitespace variants.
Verdict
LGTM. The two original findings (API-route export * parity, per-module realpathSync) and the recursive-helper gap remain resolved, and this follow-up is a clean, well-tested precision improvement to the cheap prefilter. No blocking issues.
|
Review posted to PR #2055. Summary: Verdict: LGTM — no blocking issuesRe-reviewed the new commit What I verified:
The follow-up is a clean, well-tested precision improvement. No issues to address. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review — fix(build): align Pages data export transforms (436fe62)
Re-reviewed after the CodeQL ReDoS fix in 436fe62, which replaces the comment-aware regex with a linear indexOf-based scanner in hasExportAllCandidate(). No blocking issues. Verified against the source, the prior findings, and by running the affected suites locally.
New since last LGTM: linear export-all scanner (436fe62) — correct ✅
The old regex /\bexport(?:\s|\/\*[\s\S]*?\*\/|\/\/[^\r\n]*(?:\r?\n|$))*\*/ was flagged by CodeQL (alert 206) for exponential backtracking on *//* repetitions. The replacement is a single forward pass:
- Behavioral parity with the old regex — I diffed the new scanner against the old regex across 19 inputs (tabs, CRLF, single/multiple line + block comments,
export * as ns,export*,$export, member-access.export, multiplication, unterminated comments). Zero mismatches. All realexport *forms still match;export const x = a * b,export {a}; const y = 2 * 3, andexport default a * bcorrectly returnfalse. - Word-boundary handling matches
\b— the explicit0-9 / A-Z / _ / a-zchar-code checks on the char beforeexportand after it reproduce the regex's\bsemantics, including treating$as a boundary (so$export *is a harmless false-positive in both old and new —validatePageExportsonly throws on a realExportAllDeclarationAST node, so no behavior change). - ReDoS resolved — the two CodeQL-style pathological inputs (200k
*//*, 200k unterminated block comments) plus 500k newlines all complete in <15ms; the scanner only ever advancesposition/searchFrom, so there is no backtracking path.
Prior findings remain resolved
- API-route
export *parity —validate-page-exportsstill early-returns viaisApiPage(); I confirmedisApiPagehandles rootpages/api.tsx(stripExtension("/api") === "/api") and nested/api/*, whilepages/admin/api.tsxis correctly a regular page. _app/_document/_erroranchoring —^/(?:_app|_document|_error)(?:\.[^/]*)?$verified: top-level_errorexcluded, nestedpages/admin/_error.tsxtreated as a regular page.- Per-module
realpathSyncperf — both handlers still gate canonicalization behind\0early-return → cheapcodeshort-circuit →isWithinPagesDirectorystring prefix, with results memoized inpageTransformCanonicalPaths(bounded by page-candidate count). - Recursive-helper sweep — the implementation-based fixpoint (seed →
closureChangedgrowth →pruneChangedshrink) still sweeps recursive/mutually-recursive arrow helpers; preserved by the shared-binding tests.
Verification
vp test run tests/build-optimization.test.ts tests/pages-server-exports-security.test.ts→ 150 passed (the two "Build failed" stderr lines are the expected negative-path assertions).vp check packages/vinext/src/plugins/strip-server-exports.ts packages/vinext/src/index.ts→ clean (format + lint + types).- Destructuring-assignment edits (object-pattern parenthesization, array-position preservation, computed keys), import/export attributes, regex literals, and string-with-braces all have dedicated tests.
Verdict
LGTM. The ReDoS fix is a faithful, well-tested replacement of the regex with no behavioral change, and all earlier findings stay resolved.
|
Review posted to PR #2055. SummaryVerdict: LGTM — no blocking issues. I focused on the latest commit What I verified:
The ReDoS fix is a faithful, well-tested replacement with no behavior change. |
2 participants
Summary
getServerSideProps,getStaticProps, andgetStaticPathswhile preserving shared client bindings.export *declarations and mixed SSR/SSG exports.Context
Pages client builds previously handled only a subset of Next.js's page export transform. Some export forms retained server-only dependency chains, while other forms could drop sibling or shared bindings. This change ports the relevant behavior from Next.js's page export transforms and validates the emitted client output.
This overlaps with #2052's sibling-declarator fix, but includes that case as part of the broader transform parity work.
Next.js references
test/unit/babel-plugin-next-ssg-transform.test.tscrates/next-custom-transforms/src/transforms/strip_page_exports.rspackages/next/src/build/babel/plugins/next-page-disallow-re-export-all-exports.tspackages/next/src/build/babel/loader/get-config.tspackages/next/src/build/entries.tstest/production/re-export-all-exports-from-page-disallowed/test/production/mixed-ssg-serverprops-error/Verification
vp check packages/vinext/src/plugins/strip-server-exports.ts packages/vinext/src/index.ts tests/build-optimization.test.ts tests/pages-server-exports-security.test.tsvp test run tests/build-optimization.test.ts tests/pages-server-exports-security.test.ts tests/middleware-server-only.test.ts— 153 passedvp run vinext#build