fix(app-router): ignore very dynamic requests during build analysis#1736
fix(app-router): ignore very dynamic requests during build analysis#1736NathanDrake2406 wants to merge 21 commits into
Conversation
App Router production builds currently fail when a page or route module contains guarded require(dynamic) calls. That differs from Next.js, where requests with no static request part are ignored during graph analysis and only fail if executed at runtime. The violated invariant was that bundler analysis should not turn unreachable, very dynamic requests into build-time failures. vite-plugin-commonjs tried to expand require(dynamic) as a static glob before the branch could remain runtime-only code. Add a pre-transform that preserves directives, removes only very dynamic require calls from CJS graph analysis, and marks matching dynamic import calls with @vite-ignore. The regression builds both an App Router page and route module ported from Next.js dynamic-requests coverage.
commit: |
…equest rewrite
- Parse .js/.mjs/.cjs as JSX so App Router files with JSX are not silently
skipped before the JSX-in-JS transform runs.
- Track lexical bindings and only rewrite unbound/CommonJS-style require(...).
Skips rewriting when require is declared as a local variable, function,
parameter, import specifier, or catch binding.
- Unwrap TypeScript-transparent expression nodes (TSAsExpression,
TSTypeAssertion, TSNonNullExpression, TSInstantiationExpression,
ParenthesizedExpression, ChainExpression) before classifying static parts,
ensuring expressions like require(('./' + name) as string) keep flowing
through existing analysis.
- Add focused unit and integration tests for the three blocking cases.
|
All three blocking issues are valid. Implemented fixes and pushed. Blocking issue 1 — Blocking issue 2 — name-based Blocking issue 3 — TS wrapper expressions hiding static parts: Valid. All new and existing tests in |
…uire shadowing Rewrites walkAstWithBindings as a proper two-phase scope pass: 1. Collect hoisted declarations (FunctionDeclaration, var, imports) before visiting any expressions in a scope body. 2. Walk statements and transform expressions with the full binding context already in place. This fixes two JavaScript-correctness issues: - Function declarations are hoisted, so local require is now visible throughout its enclosing scope. - Block-scoped let/const/class bindings push their own scope frames (BlockStatement, CatchClause, SwitchCase), so they never leak out and poison the parent scope. Adds tests for hoisted function declaration named require and block-scoped require not leaking to outer scope. Loosens the fast-path regex to handle comments between the callee and the paren.
… and exports The previous two-phase pre-collection passed 'block' scope type when descending into nested blocks from module/function scope. This caused two bugs: 1. TDZ false negative: a 'const require' at the top level of a function body was not collected, so a 'require(id)' before the declaration was incorrectly rewritten even though the function-scope const shadows the global. Fix: for function scope, collect all var kinds (let, const, var) at the top level of the function body — the function body itself is the enclosing block, so let/const there ARE function-scoped. 2. Block shadow leak: a 'const require' inside an 'if' or other nested block was being promoted into the enclosing module/function scope via the block-scope descent, so a 'require(id)' outside the block was incorrectly skipped. Fix: when descending from module/function scope into nested blocks, only collect 'var' (the only kind that hoists out of a block into the enclosing function/module scope). The new logic splits the two concerns: - collectDeclarationsForScope handles the top level of a scope and delegates to collectVarDeclarationsFromNestedBlocks when descending into blocks from module/function scope. - collectVarDeclarationsFromNestedBlocks walks into blocks looking ONLY for 'var' declarations (stopping at nested function boundaries). Also adds three regression tests: - const binding in function body shadows global require (TDZ) - exported function declaration named require - exported const binding named require
…lexical scope Two scope-analysis gaps surfaced by review: 1. ForStatement / ForInStatement / ForOfStatement with a let/const header introduced a per-iteration lexical scope that covered init/test/update/body (or left/right/body) but was not modeled. A 'const require' in a 'for (...)' header was therefore invisible inside the loop body, so a 'require(id)' in the body was incorrectly rewritten even though it resolved to the loop binding. Fix: when the header is a VariableDeclaration with kind 'let' or 'const', push a new scope, pre-collect those bindings (including destructuring patterns via addBindings), and walk init/test/update/ body under that scope. 'var' headers remain function-scoped and are already collected by the enclosing function-scope pre-pass, so no extra scope is pushed for them. 2. SwitchStatement cases share the lexical scope of the SwitchStatement itself unless the user wraps a case body in explicit braces. The walker previously created a fresh scope per SwitchCase, so a 'const require' in a later case did not shadow a 'require(id)' in an earlier case (the earlier case saw an empty scope and the call was rewritten). Fix: SwitchStatement now pushes one shared scope, pre-collects 'let'/'const'/'class'/'function' from every case consequent at its top level, and walks each case's consequent under that same shared scope. The standalone SwitchCase handler is retained as a defensive fallback that walks the consequent without pushing a scope, so a stray SwitchCase (parser edge case) cannot double-scope. Adds the three suggested regression tests: - for-of lexical binding shadows global require - for-initializer lexical binding shadows global require - switch-wide lexical binding shadows require in a prior case
… static block scope
Two traversal gaps surfaced by review:
1. Function parameter default RHS expressions were never walked.
'addBindings' intentionally records binding names but does not
traverse default-value expressions, so an executable
'require(dynamic)' or 'import(dynamic)' hidden in a parameter
default was invisible to the pre-transform. That left the original
build-analysis failure mode alive in code like:
function load(x = require(id)) { return x; }
Fix: after collecting parameter bindings into the function scope,
walk each parameter node. Because the bindings are already in
scope, a default like 'function f(require = require(id))' is
correctly skipped (the parameter binding named 'require' is the
resolved binding for the RHS call), while 'function load(x =
require(id))' is rewritten normally.
2. Class static blocks ('static { ... }') introduce their own
lexical scope. The generic walker previously treated them as
ordinary object recursion, so a 'const require' inside a static
block did not shadow an earlier 'require(id)' in the same block
(the earlier call was rewritten instead of resolving to the local
binding via TDZ).
Fix: handle 'StaticBlock' as a block scope. Push a new scope,
pre-collect 'let'/'const'/'class'/'function' from the static block
body, walk the body, and pop. 'var' declarations in static blocks
still hoist to the enclosing function scope and are collected by
the function-scope pre-pass.
Adds the suggested regression tests:
- unbound require(dynamic) in a function parameter default is rewritten
- unbound import(dynamic) in a function parameter default gets
@vite-ignore
- parameter default that resolves to a parameter named require is NOT
rewritten
- require shadowed by a class static block lexical binding is NOT
rewritten
…antics
Class static blocks ('static { ... }') have var-as-static-block
semantics: a 'var' declared inside a static block is scoped to the
static block itself, not hoisted to the enclosing function/module.
This differs from ordinary 'var', which is function-scoped.
The previous 'StaticBlock' handling reused the 'block' scope type,
which only pre-collects 'let'/'const' (not 'var'). That meant a
'var require' inside a static block was invisible to the static
block scope, so an earlier 'require(id)' in the same static block
was incorrectly rewritten even though it resolves to the local
'var require' binding (TDZ/undefined at runtime, not global
CommonJS require).
Fix: introduce a distinct 'static-block' scope type. It behaves
like 'function' for collection purposes — collects 'var'/'let'/
'const'/'class'/'function' at the top level and hoists 'var' from
nested control-flow blocks — but the name makes the intent clear
and keeps the two cases from being conflated.
The function/module scope pre-pass is unaffected: 'var' inside a
static block stays in the static block scope and never leaks to
the enclosing function/module scope, because the function-scope
pre-pass already does not recurse into class bodies or static
blocks.
Adds the suggested regression tests:
- 'var require' inside a static block shadows an earlier
'require(id)' in the same static block
- 'var require' inside a static block does NOT leak to an
outer 'require(id)' at the module level
…dup walker
Two correctness follow-ups and a verbosity cleanup for the
binding-aware walker.
## Fixes
### Named class expressions
A named class expression (e.g. `const C = class require { static {
require(id) } }`) binds its name in the class's own scope, and
static blocks inside see that name (per ES2022). The walker was
falling through to the generic recursion for ClassExpression, so a
`require` static block binding was not detected. Add an explicit
ClassExpression handler that:
* Walks `superClass` and decorators in the enclosing scope
(they are evaluated before the class is bound).
* Pushes a new scope, binds the class name, walks the body, and
pops the scope. Methods don't see the class name, but they
create their own function scopes when walked, so the extra
class scope is harmless for them.
* For unnamed ClassExpression, just walks the body in the
enclosing scope (the generic recursion path).
### TS `declare` declarations
A `declare const require: unknown` (or `declare function`,
``declare class`, etc.) is type-only and has no runtime binding.
OXC reports it with `declare: true` on the declaration node. The
walker was treating it as a runtime binding and skipping the
rewrite, leaving the call for the CJS plugin. Add an early
`node.declare === true` return in `collectDeclarationsForScope`
so type-only declarations never shadow the global `require`.
## Refactor
* Hoist `SCOPE_RECURSE_INTO` (the block-introducing node set)
to module level. It was duplicated in
`collectVarDeclarationsFromNestedBlocks` and
`collectDeclarationsForScope`.
* Add `forEachChild` helper for the generic child-iteration
pattern. Was duplicated in
`collectVarDeclarationsFromNestedBlocks` and the fallback
branch of `walkNode`.
* Add `isFunctionLike`, `isExportWrapper`, `isHoistingScope`
helpers. The 3-clause OR for function-like and the
export-wrapper type check were duplicated in 2-3 places each.
* Add `ScopeType` type alias. Replaces 4-fold union repetition
in function signatures.
Net effect: ~50 lines of duplication removed, ~60 lines of named
helpers (with comments) added. The walker is shorter overall and
the intent of each scope-introducing case is clearer.
## Tests
* Named class expression shadows an earlier `require(id)` in
the same static block.
* Named class expression with `extends` and a method named
`require` still shadows correctly.
* `declare const require: unknown; require(id)` rewrites the
call to the runtime helper (type-only declare does not
shadow the global require).
* Same for `declare function require` and `declare class
require`.
All 33 `ignoreVeryDynamicRequests` tests pass (was 28).
|
Addressed the latest REQUEST_CHANGES review and pushed Assessment: all three comments were valid.
Also merged current Verification:
|
|
Addressed the latest REQUEST_CHANGES review and pushed Assessment: both comments were valid.
Verification:
|
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
|
/bigbonk re-review for issues |
1 similar comment
|
/bigbonk re-review for issues |
|
/bigbonk re-review for issues |
|
@codex adversarial review |
|
To use Codex here, create an environment for this repo. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
/bigbonk re-review for issues |
|
/bigbonk review this pr for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk re-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.
Re-review for issues. The scope/binding analysis is impressively thorough and the prior review rounds resolved the substantive concerns (JSX-in-.js parsing, require shadowing across function/block/catch/static-block/labelled/switch scopes, TS transparent wrappers, declare/import type runtime-binding handling, empty-literal classification). I verified the full ignoreVeryDynamicRequests suite passes (49 tests) and vp check is clean on the plugin.
I ran a few additional adversarial probes against the actual parser. A couple of edge cases rewrite where the documented ignore_dynamic_requests boundary arguably shouldn't, but all are low-severity and rare. No blocking issues found. Details inline.
|
Review submitted successfully. Here's a summary of my re-review of PR #1736: Re-review summaryThe PR adds a Verification performed:
Findings (all non-blocking):
No blocking issues. The scope/binding analysis is thorough and well-tested. |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review for issues
Went through the plugin, the scope/binding walker, the failure-expression codegen, and the plugin ordering in index.ts. The implementation is thorough and the prior review rounds resolved the substantive concerns (JSX-in-.js parsing ordering, require shadowing across function/block/catch/static-block/labelled/switch/for-header scopes, TS transparent wrappers, declare/import type runtime-binding handling, empty-literal classification, SpreadElement arity, source-map source, dev/build parity comment). I confirmed the new plugin sits before vinext:jsx-in-js (both enforce: "pre", runs in array order) which is why parsing .js as jsx in this plugin is necessary and correct.
No blocking issues. A few non-blocking observations below for completeness — all are on unreachable/failing paths and match Turbopack's ignore_dynamic_requests framing, so they may be intentional.
|
|
||
| function isVeryDynamicImportExpression(node: AstRecord): boolean { | ||
| if (node.type !== "ImportExpression") return false; | ||
| return !requestHasStaticPart(toAstRecord(node.source)); |
There was a problem hiding this comment.
Non-blocking: isVeryDynamicImportExpression rewrites the entire ImportExpression, which discards the second argument of import(id, options) (import attributes / with). Since the expression is replaced by a rejected promise, any side effects in options are dropped. This only happens on the failing path (the import is being ignored anyway), so it matches the Turbopack "very dynamic, fails at runtime" framing — flagging for completeness, not as a defect.
Same applies to a SequenceExpression request like require((sideEffect(), id)): it has no static part, so it's rewritten and the sideEffect() is dropped. Again only on the failing path.
There was a problem hiding this comment.
Leaving this unchanged intentionally. For the ignored very-dynamic path, the contract is to replace the whole request expression with the deterministic runtime failure expression. That matches the Turbopack-style framing: no graph edge is created and execution fails at that expression rather than evaluating subexpressions that are part of an unanalyzable request. Preserving option/request side effects would make this branch less like a direct expression replacement and would add complexity for a path whose observable result is failure.
| return MODULE_EXTENSIONS.has(path.extname(cleanId)); | ||
| } | ||
|
|
||
| export function mayContainVeryDynamicRequestTarget(code: string): boolean { |
There was a problem hiding this comment.
Minor: the mayContainVeryDynamicRequestTarget fast-path regex matches import( even inside string literals or comments (e.g. a doc comment mentioning import(...)). That only forces an extra parseAst pass for those files — the AST walk then finds nothing to rewrite and returns null — so it's a perf nit, not a correctness issue. Fine as-is.
There was a problem hiding this comment.
Leaving this as-is. The regex is only a cheap precheck to avoid parsing ordinary static-import modules; false positives in comments/strings pay one parse and then the AST walk returns null. Narrowing this further would either become a lexer-lite path or risk missing real call-form targets, so the current cost/correctness boundary is intentional.
|
Review submitted. Here's a summary: Re-review summary for PR #1736The PR adds a What I verified:
Findings: No blocking issues. I left two non-blocking notes:
The implementation is high quality, the test coverage is extensive, and the adversarial review concerns from earlier rounds were all addressed. |
|
/bigbonk review for issues |
5 similar comments
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
2 participants
Overview
import(...)andrequire(...)requests.packages/vinext/src/plugins/ignore-dynamic-requests.ts,packages/vinext/src/index.ts,tests/build-optimization.test.tsrequire(dynamic)/import(dynamic)patterns that Next.js accepts.Why
App Router production builds must separate graph-analysis inputs from code that is only meaningful at runtime. Next.js/Turbopack treats requests with no static known path part as "very dynamic" and, with
ignore_dynamic_requests, avoids adding them to the module graph. Vinext violated that boundary becausevite-plugin-commonjstried to expandrequire(dynamic)as a static glob, causing a build-time failure even when the code was unreachable.require(...)calls and dynamicimport(...)expressions that have no static request part.MODULE_NOT_FOUNDthrow forrequire(...)and a promise-shapedMODULE_NOT_FOUNDrejection forimport(...), matching Turbopack codegen shape.require, localrequirebindings, and Vinext internal runtime modules untouched.What changed
require(dynamic)with exactly one argument and no static path partvite-plugin-commonjswith an invalid dynamic import error.MODULE_NOT_FOUND.import(dynamic)with no static path partMODULE_NOT_FOUND.require(),require(id, extra)require("./" + name)orimport(./${name})Maintainer review path
packages/vinext/src/plugins/ignore-dynamic-requests.tsvalidates request classification, binding handling, runtime-failure codegen, and plugin module boundaries.packages/vinext/src/index.tsplaces the plugin immediately beforevite-plugin-commonjs, which is the failing analyzer.tests/build-optimization.test.tscovers the production build boundary, scope/binding edge cases, TS wrappers,requirearity, and Vinext internal runtime exclusion.Validation
vp test run tests/build-optimization.test.ts -t ignoreVeryDynamicRequestsvp check packages/vinext/src/plugins/ignore-dynamic-requests.ts tests/build-optimization.test.tsPLAYWRIGHT_PROJECT=pages-router vp exec playwright test tests/e2e/pages-router/router-events.spec.ts tests/e2e/pages-router/navigation.spec.ts --project=pages-routertests/build-optimization.test.ts, andknipRisk / compatibility
ignore_dynamic_requestsbehaviour for requests without a static known part, including the exact-one-argumentrequireboundary.Non-goals
new URL, filesystem, or child process request patterns.References
ignore_dynamic_requestsoptionimport(...)handlingrequire(...)handlingCloses #1508