fix(build): preserve require export conditions - #2881
Conversation
|
/bigbonk review for issues |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(build): preserve require export conditions
Solid, well-factored fix. The core approach — pre-resolving literal package require() calls with Vite's explicit isRequire: true resolver before vite-plugin-commonjs hoists them into static imports — is correct, and the plugin-ordering rationale (pre-phase rewrite → normal-phase commonjs hoist) holds. The scope-aware require binding detection reuses the existing ast-scope/ast-utils helpers cleanly, and the unit tests cover the important cases (package vs relative/external/unresolved, shadowed require, independent import/require branches, and the synthetic-identity load path). Nothing here blocks merge, but a few things are worth addressing.
1. No production-build coverage (dev/prod parity)
Per AGENTS.md, request/build behavior must be verified in both dev and prod. The new tests/cjs.test.ts "conditional package exports" suite only drives startFixtureServer (dev). The whole point of this plugin is a build-time transform ordering fix, so a production build is the higher-risk path — createBuilder().buildApp() runs the multi-environment RSC/SSR/client pipeline where the \0-prefixed virtual-id re-import quirks and plugin-rsc client-reference registration behave differently than dev.
tests/helpers.ts already exposes buildAppFixture. Please add a prod-build assertion for at least the require-cjs / require-esm cases so a future regression in build mode is caught. The PR's "targeted Next.js deploy E2E" is reassuring but isn't a repo test that CI will re-run.
2. load() has no error handling and assumes moduleType: "js"
async load(id) {
const target = virtualTargets.get(id);
if (!target) return;
const file = target.split("?", 1)[0];
this.addWatchFile(file);
return { code: await readFile(file, "utf8"), moduleType: "js" };
}Two edge cases:
- The gate only checks
path.isAbsolute(...)on the require resolution. If a resolver returns an absolute id that is a virtual/proxy id (or a?commonjs-proxy-style id),readFilethrows an unhandled error, and stripping the query (target.split("?", 1)[0]) reads the base file — which may not be the content the resolver actually selected. Worth either validating the target is a real file before registering it intransform, or wrapping the read and falling back to leaving the require untouched. - A package's
requirecondition can legitimately point at a.jsonentry, in which case forcingmoduleType: "js"is wrong. Narrow, but consider deriving the module type from the target extension.
3. virtualTargets grows unbounded
The Map is module-scoped and never pruned across the dev server lifetime. In practice it's bounded by the number of distinct divergent packages so it's not a real leak, but it's shared across all environments (RSC/SSR/client) via the single plugin instance. That sharing is actually fine (keys are absolute-path-derived, so all environments resolve to the same target), but a one-line comment noting the intentional cross-environment sharing would help the next reader, since it looks surprising next to the "RSC and SSR are separate module graphs" gotcha in AGENTS.md.
4. Divergence check compares full resolutions including query
requireResolution === importResolutionIf require/import resolve to the same file but with differing query suffixes (e.g. ?v=), this treats them as divergent and mints an unnecessary synthetic module. Comparing on the query-stripped path (as you already do for the path.isAbsolute check) would avoid that. Minor, but cheap to tighten.
Nits
TRANSFORMABLE_ID_RE+LITERAL_REQUIRE_REwill re-run this transform on the loaded synthetic module itself, recursively handling nestedrequire()s. That looks intentional/correct, but it's implicit — a brief comment would confirm it's by design rather than accidental.
Overall: correct fix, good tests for the unit surface. The main thing I'd want before merge is a production-build assertion (#1); the rest are hardening.
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
Review posted to PR #2881. SummaryI reviewed PR #2881 ( Findings I posted (none blocking, but #1 is the main ask before merge):
Plus a nit about the transform recursively re-processing the synthetic module (looks intentional, deserves a comment). |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: fix(build): preserve require export conditions
Re-reviewed at 8c128b8. The four findings from the previous review plus the nit have all been addressed, and I did an independent pass over the new hardening. No blocking issues; the fix is correct and well-tested.
Prior findings — resolved
- Prod-build coverage (was the main ask): ✅
tests/cjs.test.tsnow runsbuildAppFixture(root)+startProdServer(...)and asserts all four conditional-export cases (import-cjs/require-cjs/import-esm/require-esm) in both dev and production (CONDITIONAL_EXPORT_CASES× twoit.eachblocks). This is the higher-risk path for a build-time transform-ordering fix, so this closes the dev/prod-parity gap. Verified locally: 11/11 pass. load()error handling + module type: ✅load()now wrapsreadFileand defersENOENT/EISDIRto Vite's contextual loader (rethrowing anything else), andmoduleTypeis derived from the target extension viasyntheticModuleType(js/jsx/json/ts/tsx). New unit tests cover the JSON-typing and stale-target-deferral paths.virtualTargetscross-env sharing: ✅ Comment added (lines 219–222) explaining the map is intentionally shared across RSC/SSR/client and bounded by distinct conditional targets.- Divergence check ignoring query: ✅ Now compares
stripViteModuleQuery(requireResolution)vs the query-stripped import path, with a dedicated "ignores query-only differences" test.
Nit (recursive re-processing of the synthetic module) is also now documented at lines 265–266.
Independent verification of the new code
- Nested requires in the synthetic module resolve correctly. When the
.vinext-require.jsmodule re-enterstransform, the importer id is the synthetic path, but it shares the real target's directory, sonode_modulesresolution walks the same tree..vinext-require.jsonsynthetic ids are correctly excluded from re-transform byTRANSFORMABLE_ID_RE(no.json), so JSON is never parsed as JS. commonjsTransformFiltergating is sound. It returnstrueonly for.vinext-require.{js,jsx,ts,tsx}ids (forcing CJS conversion of the synthetic module) and preserves the existingfalseskip for project-local.cjs/.cts. JSON synthetic ids fall through toundefined, which is correct.- Shadowed-
requirescope detection is correct for the CJS-wrapper case: a freerequirein a CJS module has no lexical binding, so it's rewritten; aconst require = ...at module scope is captured bycollectDirectScopeBindingsand left alone — matching webpack/Next.js static-analysis behavior. - Wrapper preservation:
unwrapExpression+overwrite(argument.start, argument.end, ...)replaces only the inner literal, sorequire(("lib"))/require("lib" as string)keep their wrappers intact.
vp check is clean on the changed files; full CI (Check, all Vitest shards, all E2E) is green.
Optional follow-ups (non-blocking)
- The recursive-re-processing behavior is covered indirectly by integration tests but has no direct unit test for a nested divergent require inside a synthetic module. If you want to lock in that contract, a focused unit test (synthetic module whose CJS body itself
require()s a second divergent package) would be cheap insurance. virtualTargetsis still never pruned across a long-lived dev server. As noted, it's bounded by distinct divergent packages so it's not a practical leak — leaving as-is is fine.
LGTM.
|
Review posted to PR #2881. SummaryI re-reviewed PR #2881 ( All four prior-review findings + the nit are resolved:
My independent checks (no issues):
Verified locally: unit tests 9/9, cjs integration 11/11, Two optional non-blocking follow-ups noted (a direct unit test for nested divergent requires; unbounded-but-harmless |
|
Independent targeted Next.js E2E verification on immutable head
The checkout remained clean and the local/remote PR head SHA matched after the run. |
|
Independent full-diff review of immutable head I reviewed all four changed files independently of Big Bonk, covering literal package Correctness/parity checks:
Independent validation on this exact SHA:
The local and remote heads still match, the worktree is clean, all checks are green, and the PR is mergeable. |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(build): preserve require export conditions
Reviewed at merged head 78f5a5a (PR base main). The core mechanism — pre-resolving literal package require() calls with Vite's explicit isRequire: true resolver before vite-plugin-commonjs hoists them into static imports, then minting a synthetic .vinext-require.* identity for divergent require/import targets so CJS conversion and plugin-rsc still run — is correct and well-factored. Plugin ordering holds: createRequireConditionResolutionPlugin() is enforce: "pre" and registered before commonjs(...), while @vitejs/plugin-rsc is pushed last, so the pre-resolution runs first, the commonjs hoist next, and plugin-rsc's ESM proxy injection last.
I independently re-verified the four prior findings and the nit are resolved, and did a fresh pass over the current code. No blocking issues; LGTM.
Verification
- Unit tests:
tests/require-condition-resolution.test.ts— 9/9 pass. - Integration (dev + prod):
tests/cjs.test.ts— 11/11 pass, all four conditional-export cases (import-cjs/require-cjs/import-esm/require-esm) in both dev and production. vp checkon both changed source/test files — clean (format, lint, types).
Independent correctness checks (all sound)
moduleTypetyping.load()returns"js" | "jsx" | "ts" | "tsx" | "json". Rolldown'sSourceDescription.moduleType(theload-hook type) is"js" | "jsx" | "ts" | "tsx" | "json" | ..., so all five are valid —vp checkconfirms. (Note this differs from the napiResolveResult.ModuleTypeunion, which is the narrower'module' | 'commonjs' | 'json' | 'wasm' | 'addon'; the load hook uses the wider one.)- Builtin/self requires are skipped.
require("fs")resolves to"fs"and is caught byrequireResolution === specifier;require("node:fs")is excluded by the protocol regex inisPackageSpecifier. Windows drive-absolute (C:\...) is also excluded by the same regex. - Nested require resolution. When the synthetic
<abs>.vinext-require.jsre-enterstransform, the importer id shares the real target's directory, sonode_modulesresolution walks the correct tree; thecodefilter (\brequire\s*\() only re-triggers when the synthetic body actually contains arequire(..jsonsynthetics are excluded from re-transform byTRANSFORMABLE_ID_RE. commonjsTransformFiltergating. Returnstrueonly for.vinext-require.{js,jsx,ts,tsx}ids, preserves the existingfalseskip for project-local.cjs/.cts, and lets everything else fall through toundefined.createIdResolvercontainer. Only runs alias + Vite's own resolver, so it cannot emit?commonjs-proxy/virtual ids — the theoretical "readFile reads the base file, not the resolved proxy content" concern from the first review is not reachable here.- Scope-aware
requiredetection reuses the existingast-scopehelpers and correctly leaves lexically-boundrequireuntouched (unit-tested).
Non-blocking observations (optional)
.mjs/.mtsrequire targets are forced through the CJS filter.syntheticModuleTypemaps.mts→"ts", so an ESMrequire-condition target yields a.vinext-require.tsid thatcommonjsTransformFilterforces totrue(CJS conversion). In practice divergent-require targets are CJS, andvite-plugin-commonjspasses already-ESM source through, so this is harmless — but if you ever hit a package whoserequirecondition points at genuine ESM, worth a note that the filter forces conversion regardless of the target's actual module format.virtualTargetsstill unbounded across a long-lived dev server (bounded by distinct divergent packages; already documented as intentional — fine to leave).- No direct unit test for a nested divergent require inside a synthetic module. Covered indirectly by integration, but a focused unit test would lock in the contract cheaply (already noted as an optional follow-up in the prior review).
Overall: correct fix, good dev/prod coverage, prior findings resolved.
|
Review posted to PR #2881. SummaryI reviewed PR #2881 ( No blocking issues; LGTM. All four prior review findings plus the nit are resolved, and my independent pass found no defects. Verification:
Independent correctness checks (all sound):
Non-blocking observations noted: |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(build): preserve require export conditions
Reviewed at head ce61c6e. The core mechanism — pre-resolving literal package require() calls with Vite's explicit isRequire: true resolver before vite-plugin-commonjs hoists them into static imports, then minting a synthetic .vinext-require.* identity for divergent require/import targets — is correct and well-factored. Plugin ordering holds (enforce: "pre" require-condition plugin → commonjs() → plugin-rsc last), and the scope-aware require binding detection reuses the existing ast-scope/ast-utils helpers cleanly.
I independently re-verified that the four prior findings + the nit are resolved, and did a fresh pass over the current code. No blocking issues.
Verification
- Unit:
tests/require-condition-resolution.test.ts— 9/9 pass. - Integration (dev + prod):
tests/cjs.test.ts— 11/11 pass, all four conditional-export cases in both dev and production. vp checkclean on all three changed files (format, lint, types).
Independent correctness checks (all sound)
- Divergence gating.
importResolution === undefinedwith a resolvedrequireResolutioncorrectly mints a synthetic (they genuinely diverge). Query-only differences are stripped before comparison, so?import/?requiresuffixes on the same file don't mint a spurious module. - Nested resolution. The synthetic id
${requirePath}.vinext-require.jsshares the real target's directory, so re-entrantnode_moduleswalks resolve correctly;.jsonsynthetics are excluded from re-transform byTRANSFORMABLE_ID_RE. require.resolve(...)/ member-callee and lexically-boundrequireare correctly left untouched (member callee isn'tIdentifier "require"; scope walk covers function/block/loop/switch/catch/class-expression/TS wrappers).- Specifier classification.
#imports, scoped (@scope/pkg), subpath (lib/sub),node:-protocol, and Windows drive specifiers are all handled/excluded correctly byisPackageSpecifier. - Graceful parse degradation. A
.js/.cjsfile with TS-generic-call-shaped syntax (foo<Bar>()) failsparseAst({ lang: "jsx" }), butcollectLiteralRequiresswallows the throw and returns[], leaving the require untouched (falls back to Vite default) rather than crashing.
Non-blocking observations
-
Transform has no
node_modulesguard (performance). Thetransformhook filters onid: TRANSFORMABLE_ID_RE+code: /\brequire\s*\(/, but does not excludenode_modules. Many transpiled dependencies contain therequire(substring, so this adds a fullparseAst+ scope walk to a potentially large number of dependency modules on every dev request and every build. This matches the AGENTS.md performance guidance ("avoid broad compatibility transforms overnode_modules… and modules that cannot contain the syntax being rewritten") and is a plausible contributor to the +5.2% production build-time regression flagged in the benchmark comment (single-run, so partly noise, but directionally consistent). The transform genuinely must run on somenode_modulesfiles (that's where conditional-exportrequire()s live), so a blanket exclusion isn't right — but it's worth confirming the added parse cost is acceptable, and considering whether prebundled/optimized deps can be skipped. -
ESM require-condition target forced through the CJS filter.
syntheticModuleTypemaps.mjs→js/.mts→ts, so arequirecondition that points at genuine ESM yields a.vinext-require.{js,ts}id thatcommonjsTransformFilterforces totrue(CJS conversion). Harmless in practice — divergent-require targets are CJS, andvite-plugin-commonjspasses already-ESM source through — but the filter forces conversion regardless of the target's actual module format. -
virtualTargetsis unbounded across a long-lived dev server (bounded by distinct divergent packages; already documented as intentional — fine to leave). -
No direct unit test for a nested divergent require inside a synthetic module. Covered indirectly by the dev/prod integration cases; a focused unit test would lock in the recursive-reprocessing contract cheaply.
Overall: correct fix, good dev/prod coverage, prior findings resolved. The only item I'd flag for a look before merge is the transform's performance scope (#1); the rest are optional.
|
Review posted to PR #2881. SummaryI reviewed PR #2881 ( No blocking issues. Verified locally: unit 9/9, dev+prod integration 11/11, The one new observation worth a look before merge:
Plus three optional non-blocking items: ESM require-targets forced through the CJS filter (harmless), unbounded My independent correctness checks — divergence gating, nested resolution, |
|
Final exact-head refresh validation:
Against baseline run |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(build): preserve require export conditions
Re-reviewed at head a5fc4d1, which adds one new commit since the last pass — perf(build): skip ineligible require condition scans. That commit directly addresses the performance finding (#1) from the prior review (the transform having no node_modules guard). No blocking issues; LGTM.
The new guard is correct, not just faster
The new commit threads commonjsTransformFilter into the require-condition plugin and gates the transform:
const commonjsDisposition = commonjsTransformFilter?.(cleanId);
if (
commonjsDisposition === false ||
(commonjsDisposition !== true && cleanId.includes("node_modules"))
) {
return null;
}I verified this is a faithful mirror of vite-plugin-commonjs@0.10.4's own transform gate (dist/index.js:410-413):
if (userCondition === false) return;
if (userCondition !== true && id.includes("node_modules")) return;This is the key correctness point: there is no value in pre-resolving a require's export condition unless vite-plugin-commonjs is actually going to hoist that require() into a static import (which is what flips the resolution from require-kind to import-kind). By gating on the exact same predicate, the plugin now rewrites requires in precisely the set of files the commonjs pass will process — no more, no less. Ordinary node_modules files fall through to vite-plugin-commonjs's own skip, where Vite/Rolldown already preserves require conditions natively. So this is a safe narrowing, not a behavioral change to the fix.
The recursive path is preserved: a synthetic .vinext-require.js id returns true from the filter (isConditionalRequireScriptModuleId), so the commonjsDisposition !== true clause short-circuits and nested divergent requires inside the synthetic still recurse — even though the synthetic id itself lives under node_modules. The new "rewrites nested package requires in synthetic targets" unit test locks this contract in, which also closes the prior optional follow-up (#4) about missing direct coverage for nested synthetic requires.
The top-level fixture requires (app/require-cjs/page.tsx, app/require-esm/page.tsx) are first-party files (not in node_modules), so the guard does not touch them — confirmed by the still-passing dev+prod integration cases.
Windows / slash handling
cleanId here is stripViteModuleQuery(id) without toSlash, but that's fine: .includes("node_modules") is slash-direction-agnostic (the substring survives either separator), and vite-plugin-commonjs's upstream gate is likewise unnormalized. commonjsTransformFilter internally toSlashes before its own regex checks, so the disposition value is consistent across platforms.
Verification (at a5fc4d1)
tests/require-condition-resolution.test.ts— 13/13 pass (was 9; +4 for the node_modules skip, project-.cjs/.ctsskip, and nested-synthetic cases).tests/cjs.test.ts— 11/11 pass, all four conditional-export cases in both dev and production.vp checkon all four changed files — clean (format, lint, types).
Non-blocking observations (unchanged, optional)
- ESM require-condition target forced through the CJS filter.
syntheticModuleTypemaps.mjs→js/.mts→ts, so arequirecondition pointing at genuine ESM yields a.vinext-require.{js,ts}id thatcommonjsTransformFilterforces totrue. Harmless in practice (divergent-require targets are CJS, andvite-plugin-commonjspasses already-ESM source through), but the filter forces conversion regardless of the target's actual module format. virtualTargetsunbounded across a long-lived dev server (bounded by distinct divergent packages; documented as intentional — fine to leave).
Overall: the new commit resolves the last flagged item cleanly by aligning the pre-resolution scope with the downstream commonjs gate. Correct fix, good dev/prod coverage, all prior findings resolved.
|
Review posted to PR #2881. SummaryI reviewed PR #2881 ( Key verification I did this pass:
Verified locally: unit tests 13/13 (up from 9), cjs dev+prod integration 11/11, Two unchanged non-blocking observations noted ( |
1 participant
Summary
Failure mapping
This fixes the two non-cache failures in Next.js deploy run 31439707085, Test report job 93624401572:
Validation
Existing PR #2334 was not adopted because it conflicts with main.