fix(app-router): match dynamic request analysis parity

[james-elicx]

Summary

• match Next.js/Turbopack analysis for fully dynamic require() and import() expressions
• preserve statically analyzable aliases, conditionals, templates, lexical scopes, and explicit ignore directives
• emit deterministic runtime failures for unsupported dynamic requests without evaluating unsafe expressions

Next.js parity

Targets the dynamic-request failures in the latest Next.js deploy-suite report, while matching pinned Next.js/Turbopack v16.2.6 behavior.

Validation

• tests/dynamic-requests-build.test.ts: 21/21 passed with one worker
• scoped vp check
• vp run vinext#build
• repeated independent review loop: no actionable findings

Full suites are deferred to CI.

[james-elicx]

/bigbonk review for issues

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2253

npm i https://pkg.pr.new/vinext@2253

commit: 6329511

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original
workers-cache	preview	production
web	preview	production

[ask-bonk]

Review: dynamic request analysis parity (PR #2253)

Solid, well-tested implementation. The scope tracking, constant-folding, ignore-directive parsing, and webpack/turbopack-style runtime stubs all look correct, and the fixture faithfully ports the Next.js v16.2.6 dynamic-requests e2e test (app/page.tsx, app/hello/route.ts, app/layout.tsx). 21/21 tests pass locally and vp check is clean. The bulk of the logic is sound — comments below are one performance concern plus minor nits.

Performance: no cheap content guard before full AST parse (ignore-dynamic-requests.ts:563)

The transform handler filters only by file extension (filter.id.include), so it runs parseAst() + a full recursive scope/AST walk on every JS/TS module in the graph — all user code and all node_modules dependencies — even when the file contains no require( / import( at all. Because the plugin is registered in the shared PluginOption[], this happens across the client, SSR, and RSC environments (up to ~3x per module).

This is exactly the pattern the repo's own performance guidance warns against:

Avoid broad compatibility transforms over node_modules, generated runtimes, prebundles, and modules that cannot contain the syntax being rewritten.

A cheap substring pre-check before parsing would skip the overwhelming majority of files at near-zero cost while preserving behavior, e.g.:

handler(code, id) {
  // fast path: nothing to do if the file can't contain a dynamic request
  if (!code.includes("require") && !code.includes("import")) return null;
  ...
}

(import(/require( can be separated by comments, so a loose includes on the bare identifiers is the safe form.) There's also no exclusion for Vite's pre-bundled deps (.vite/deps) or \0 virtual modules — worth considering alongside the content guard.

Minor: resolvingBindings not threaded into predicate evaluation

In requestHasStaticPart, the ConditionalExpression/LogicalExpression branches call staticTruthiness(node.test, scope) / staticNullishness(node.left, scope) without forwarding the current resolvingBindings set (ignore-dynamic-requests.ts:286, :297, :305). It's not a correctness bug — those helpers run their own cycle detection from a fresh set — but it's inconsistent with the rest of the file and lets a predicate re-walk a binding already being resolved. Worth threading through for consistency.

Minor: redundant extension matching

filter.id.include (/\.[cm]?[jt]sx?.../) matches non-existent extensions like .cjsx/.mtsx that TRANSFORMABLE_EXTENSIONS then rejects (:565). Harmless redundancy; could tighten the regex to match the set if you want them to agree.

None of these block correctness — the performance guard is the one I'd recommend addressing before merge given the repo's explicit guidance on graph-wide transforms.

[ask-bonk]

Review posted to PR #2253. Summary of findings:

Overall: Correct, well-tested implementation. Scope tracking, constant-folding, ignore-directive parsing, and the webpack/turbopack-style runtime stubs are all sound. The fixture faithfully ports the Next.js v16.2.6 dynamic-requests e2e test. 21/21 tests pass locally; vp check clean.

Findings:

1. Performance (main concern) — ignore-dynamic-requests.ts:563: The transform filters only by file extension, so it runs parseAst() + a full recursive AST/scope walk on every JS/TS module in the graph (all user code and all node_modules), across client/SSR/RSC environments, even for files with no dynamic requests. This is exactly the graph-wide-transform pattern AGENTS.md's performance guidance warns against. A cheap code.includes("require")/includes("import") guard before parsing would skip the vast majority of files. No .vite/deps / virtual-module exclusion either.

2. Minor — resolvingBindings not threaded into staticTruthiness/staticNullishness calls in the conditional/logical branches (:286, :297, :305). Not a correctness bug (those have their own cycle detection) but inconsistent.

3. Minor — redundant extension matching between the filter.id regex and TRANSFORMABLE_EXTENSIONS.

Only the performance guard is worth addressing before merge; the rest are nits.

github run

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: dynamic request analysis parity (PR #2253)

Reviewed the full diff (ignore-dynamic-requests.ts, the index.ts registration, and the test fixture) against the current HEAD 3d6c01f. The implementation is correct and well-tested — 21/21 pass locally and vp check is clean on the plugin file. Below is the status of every flagged item plus the one finding I'd still address before merge.

CodeQL alerts (218/219/220) are stale — already fixed at HEAD

All three GitHub Advanced Security alerts ran against commit 072c0cf, the commit referenced in the pkg-pr-new comment:

• 218 "Inefficient regular expression" — flagged the trivia regex /\((?<trivia>(?:\s|\/\*[\s\S]*?\*\/|\/\/[^\r\n]*(?:\r?\n|$))*)$/ (nested (?:...)* wrapping \/\*[\s\S]*?\*\/ — the classic (/* … *//* exponential-backtracking shape).
• 219 / 220 "Improper code sanitization" — same dataflow, tracing the regex-derived leadingArgumentTrivia value into the comment parser.

The latest commit 3d6c01f ("parse dynamic request comments safely") removed that regex entirely and replaced it with a linear indexOf/startsWith scanner (hasDynamicRequestIgnoreDirective, :85–:138). I confirmed HEAD contains no .match/.matchAll/new RegExp/new Function/eval, so there is no ReDoS or code-construction surface left. These alerts should clear on the next CodeQL run; no action needed beyond a re-scan.

Performance: no cheap content guard before the full AST parse (:608)

This is the one I'd address before merge. The transform hook filters only by extension (filter.id.include), so transformVeryDynamicRequests runs parseAst() + a full recursive scope/AST walk on every JS/TS module — all user code and all node_modules deps — even when the file contains no require(/import(. Because the plugin is in the shared PluginOption[] (index.ts:1134) with no applyToEnvironment restriction, this happens in the client, SSR, and RSC environments (~3x per module).

This is exactly the pattern AGENTS.md's performance guidance warns against ("Filter Vite hooks before JavaScript runs… Avoid broad compatibility transforms over node_modules…"), and the repo already establishes the fix pattern — cheap code.includes(...) guards before parsing in index.ts:1194, plugins/optimize-imports.ts:202, and plugins/import-meta-url.ts:223. Recommend the same here, before parseAst:

// fast path: nothing to rewrite if the file can't contain a dynamic request
if (!code.includes("require") && !code.includes("import")) return null;

(Loose bare-identifier includes is the safe form, since require/import can be separated from ( by comments.) Note: parsing dependency code is intentional for Next.js parity — the dependency: true fixture (:368, :409) verifies deps are rewritten — so the guard should stay a content pre-check, not a node_modules exclusion.

Minor: resolvingBindings not threaded into predicate evaluation (:331, :342, :350)

The ConditionalExpression/LogicalExpression branches call staticTruthiness(node.test, scope) / staticNullishness(node.left, scope) without forwarding the active resolvingBindings set. Not a correctness bug — those helpers seed their own cycle detection — but it lets a predicate re-walk a binding already mid-resolution and is inconsistent with the threaded calls elsewhere in the file. Cheap to thread through.

Minor: redundant extension matching (:605 vs :610)

filter.id.include (/\.[cm]?[jt]sx?(?:\?.*)?$/) admits non-existent extensions like .cjsx/.mtsx that TRANSFORMABLE_EXTENSIONS then rejects at :610. Harmless; the two could be tightened to agree if you care.

---

Verdict: Correctness and parity look good; the fixture faithfully ports the Next.js v16.2.6 dynamic-requests e2e test. The CodeQL alerts are already resolved at HEAD. The only thing I'd land before merge is the content guard — everything else is optional polish.

[ask-bonk]

Review posted to PR #2253. Summary of what I found:

Findings

1. CodeQL alerts (218/219/220) are stale — already fixed at HEAD.
All three GitHub Advanced Security alerts ran against the older commit 072c0cf. They flagged the trivia-parsing regex /\((?<trivia>(?:\s|\/\*[\s\S]*?\*\/|...)*)$/ — a genuine ReDoS shape (nested quantifiers wrapping a comment match) plus its derived "improper code sanitization" dataflow. The latest commit 3d6c01f ("parse dynamic request comments safely") removed that regex entirely and replaced it with a linear indexOf/startsWith scanner. I confirmed HEAD has no .match/.matchAll/new RegExp/new Function/eval, so the alerts should clear on re-scan. No action needed.

2. Performance (the one I'd address before merge) — ignore-dynamic-requests.ts:608: the transform filters only by extension, so it runs parseAst() + a full recursive AST/scope walk on every JS/TS module (user code and node_modules), across all 3 environments, even for files with no dynamic requests. AGENTS.md explicitly warns against this; the repo already uses cheap code.includes(...) guards elsewhere. Recommended a code.includes("require")/("import") pre-check. Noted that parsing dependency code is intentional for parity (the dependency: true fixture proves it), so the fix is a content guard, not a node_modules exclusion.

3. Minor — resolvingBindings not threaded into staticTruthiness/staticNullishness in the conditional/logical branches (:331, :342, :350). Consistency nit, not a correctness bug.

4. Minor — redundant extension matching between the filter.id regex and TRANSFORMABLE_EXTENSIONS.

Verdict: correctness/parity are solid, 21/21 tests pass locally, vp check clean. The CodeQL alerts are already resolved at HEAD; the content guard is the only thing worth landing before merge.

github run

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: dynamic request analysis parity (PR #2253)

Reviewed the full diff at HEAD 6329511 (ignore-dynamic-requests.ts, the index.ts registration, and tests/dynamic-requests-build.test.ts). I re-ran the build/test suite locally — 21/21 pass — and vp check is clean on the plugin file. Correctness and Next.js v16.2.6 parity look good. Verdict: no blocking issues; the prior review's two main asks are already resolved at HEAD.

Previously-flagged items are resolved at HEAD

• CodeQL alerts 218/219/220 (ReDoS + improper code construction) ran against the older commit 072c0cf and flagged the trivia-parsing regex. The follow-up commit 3d6c01f ("parse dynamic request comments safely") removed that regex entirely and replaced it with a linear indexOf/startsWith scanner (hasDynamicRequestIgnoreDirective, ignore-dynamic-requests.ts:70–139). HEAD contains no .match/new RegExp/new Function/eval, so the ReDoS/code-construction surface is gone. These alerts should clear on re-scan; no action needed.
• Performance content guard — the earlier review recommended a cheap code.includes(...) pre-check before parseAst. That guard now exists at ignore-dynamic-requests.ts:474:

  if (!code.includes("require") && !code.includes("import")) return null;

  This skips the full AST/scope walk for any module that can't contain a dynamic request, matching the repo's performance guidance. Parsing dependency code remains intentional for parity (the dependency: true fixture verifies deps are rewritten), so a content guard rather than a node_modules exclusion is the right call.

Minor (non-blocking): resolvingBindings not threaded into predicate evaluation

In requestHasStaticPart, the ConditionalExpression/LogicalExpression branches call staticTruthiness(node.test, scope) / staticNullishness(node.left, scope) without forwarding the active resolvingBindings set (:331, :342, :350). This is not a correctness or termination bug — both helpers seed their own fresh cycle-detection set (:214, :252) and bindings are finite, so they always terminate. It just lets a predicate re-walk a binding already mid-resolution and is inconsistent with the threaded calls elsewhere in the file. Cheap to thread through if you want consistency.

Minor (non-blocking): redundant extension matching

filter.id.include (/\.[cm]?[jt]sx?(?:\?.*)?$/, :607) admits non-existent extensions like .cjsx/.mtsx that TRANSFORMABLE_EXTENSIONS then rejects at :612. Harmless redundancy; the two could be tightened to agree.

Notes / things I checked that are fine

• The plugin is enforce: "pre" and registered before commonjs() (index.ts:1134). Replacing fully-dynamic require() with a runtime MODULE_NOT_FOUND throw before the CJS transform is correct — webpack/turbopack also fail these, and the rewrite only fires when the request has no statically known path part.
• Static ImportDeclaration and import.meta are untouched (the visitor only acts on ImportExpression and require CallExpression).
• Nested require(require(x)) replaces the outer expression and skips the inner — acceptable, matches webpack treating the whole expression as unresolvable.
• void-argument requests are stubbed without evaluating side effects (test at :220 verifies calls === 0).

Bottom line: correctness, parity, and the two earlier high-priority items (ReDoS regex and the perf guard) are all addressed at HEAD. The remaining items are optional polish. LGTM.

[ask-bonk]

Review posted to PR #2253.

Summary

I reviewed the full diff at HEAD 6329511 against main, re-ran the test suite (21/21 pass), and ran vp check (clean). Findings:

Previously-flagged items already resolved at HEAD:

• CodeQL ReDoS / code-construction alerts (218/219/220) — flagged against the older commit 072c0cf. Commit 3d6c01f removed the offending trivia regex entirely and replaced it with a linear indexOf/startsWith scanner. No .match/new RegExp/new Function/eval remains. Should clear on re-scan.
• Performance content guard — the requested code.includes("require")/("import") pre-check before parseAst now exists at ignore-dynamic-requests.ts:474, addressing the graph-wide-transform concern from AGENTS.md.

Minor / non-blocking:

• resolvingBindings not threaded into staticTruthiness/staticNullishness in the conditional/logical branches (:331, :342, :350) — consistency nit, not a correctness or termination bug (those helpers seed their own cycle detection).
• Redundant extension matching between the filter.id regex and TRANSFORMABLE_EXTENSIONS.

Verified fine: enforce: pre ordering before commonjs(), static imports/import.meta untouched, nested-require handling, and void-argument side-effect safety.

No blocking issues. The two high-priority items from earlier reviews (ReDoS regex and perf guard) are both addressed at HEAD.

github run

[github-actions]

Performance benchmarks

Compared 6329511 against base cf9d601 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 2 regressed · 4 within ±1.5%

Scenario	Framework	Baseline	Current	Change
Client bundle size (gzip)	vinext	132.8 KB	132.8 KB	⚫ -0.0%
Client entry size (gzip)	vinext	118.7 KB	118.7 KB	⚫ -0.0%
Dev server cold start	vinext	2.35 s	2.49 s	🔴 +6.0%
Production build time	vinext	2.84 s	3.03 s	🔴 +6.8%
RSC entry closure size (gzip)	vinext	92.2 KB	92.2 KB	⚫ +0.0%
Server bundle size (gzip)	vinext	166.9 KB	166.9 KB	⚫ +0.0%

View detailed results and traces

_{🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head}