fix(pages): preserve CJS globals in bundled dependencies - #2826
fix(pages): preserve CJS globals in bundled dependencies#2826james-elicx wants to merge 2 commits into
Conversation
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
0a4cfb8 to
cb39205
Compare
|
/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 |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
/bigbonk review — PR #2826 (fix(pages): preserve CJS globals in bundled dependencies)
Reviewed the full diff, ran the touched tests, and traced the dev vs build code paths. This looks solid and well-scoped. Summary and a few observations below.
What I verified
tests/import-meta-url.test.ts— 50 passedtests/pages-router.test.ts(bundled-CJS + relocated-prod) — passed; the relocated build assertsimport.meta.dirnamein the server chunk and that the runtime path resolves relative to the copied output (.not.toContain("/node_modules/")), which is the key relocatability guarantee.tests/build-optimization.test.ts— 149 passedvp checkonimport-meta-url.ts+index.ts— clean
Design is correct and consistent
- Dev/serve path bakes canonical absolute
__filename/__dirnameat transform time (rewriteBundledDependencyCjsGlobals) — fine for a running dev server. - Build path deliberately does not bake absolute paths: the serve plugin is gated
enabled: () => isServeCommand, so in build onlyrenderChunkinjectsvar __dirname = import.meta.dirname, keeping the emitted chunk relocatable. This matches Next's webpack Pages behavior (chunk-relative), and the relocated-prod test proves it. - The
commonjsfilter change is correct against vite-plugin-commonjs 0.10.4 semantics (false= skip,true= force even for node_modules,undefined= default). Returningtrueonly forisBundledCommonJsDependencyId+isCommonjs(code)is appropriately narrow. analyzeServerCjsGlobalsguards against a duplicate top-levelvar __dirnamecollision in a mixed chunk viamoduleBindings, so therenderChunkinjection won't emit a redeclaration.- Cloudflare/Nitro/App-Router/client are all gated off. This is a deliberate, documented scope (
__dirnamein a Worker has no meaningful filesystem value), consistent with the PR description.
Minor observations (non-blocking)
-
E2E host inconsistency.
tests/e2e/pages-router-prod/cjs-dependency-globals.browser.spec.tsuseshttp://127.0.0.1:4175, while every sibling spec in that dir useshttp://localhost:4175. Functionally equivalent, but worth aligning for consistency (thePAGES_ROUTER_PROD_BASE_URLenv fallback is otherwise unused by the other specs). -
mergeOptimizeDepsExcludebroadening is scope-adjacent. The SSRexcludegrew from["ipaddr.js"]to includereact,react-dom,react-dom/server, and allnextShimMapkeys. This is coherent with the "don't enable env-wide discovery" goal and is asserted by the new test, but it's a broader optimizer behavior change than the CJS-globals fix strictly needs. Fine as-is; just flagging that it's the highest-surface part of the diff. -
isCommonJsDependencydoes syncfspackage.json walks per matching dependency module. Gated behind the nativecode: /__filename|__dirname/+ node_modules id filter and Vite's per-module transform caching, so this only fires for the rare dep that actually reads these globals — acceptable, no change needed.
No correctness issues found
Dev/prod parity holds (build path goes through renderChunk for all Pages server builds via buildApp), the relocatability contract is tested, and the fix matches Next.js Pages-bundle behavior. LGTM.
|
Review posted to PR #2826. SummaryI reviewed PR #2826 ( Verdict: LGTM — no correctness issues found. Key findings:
Three non-blocking observations flagged: an E2E host inconsistency ( |
cb39205 to
d1bab88
Compare
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review — PR #2826 (fix(pages): preserve CJS globals in bundled dependencies)
Traced the dev/build/optimizer code paths, verified the vite-plugin-commonjs internals the wrapper depends on, and ran the touched suites. Verdict: LGTM — no correctness issues found. A couple of minor/latent-fragility notes below, none blocking.
What I verified
tests/import-meta-url.test.ts— 55 passedtests/build-optimization.test.ts— 149 passed (2 skipped)tests/pages-router.test.ts -t "bundled"— 3 passed (real dev route + relocated-prod build assertingimport.meta.dirnamein the server chunk and.not.toContain("/node_modules/"))vp checkonimport-meta-url.ts+index.ts— clean
Design is correct
- The sync-filter bridge is sound. I confirmed against
vite-plugin-commonjs@0.10.4thattransformreturnstransformCommonjs(...), whose firstawait(dynaimcRequire.generateRuntime) comes after theoptions.filter?.call(id)invocation. SotransformBundledCommonJsDependenciesis guaranteed to still be set when the filter reads it, and thefinallyreset after the sync prefix is safe. The "do not await here" comment is accurate. - Dev vs build split is right. Dev node_modules transform bakes source-path (or Edge) globals; in build the node_modules branch bails on
mode !== "dev"andrenderChunkinjects chunk-relativeimport.meta.dirname(Node/Nitro) or/index.js///(Worker). The relocated-prod and real Nitro/workerd tests prove each variant. - Idempotency / no double-injection.
analyzeServerCjsGlobalsrecords module-scope bindings and skips injection when__dirname/__filenameis already bound, so a prebundled dep already rewritten by the optimizer plugin, or a chunk where rolldown's CJS interop declared the global, won't get a redeclaration. Function-param__dirnamein a CJS wrapper is correctly not treated as a module binding (a harmless unused top-levelvarresults, shadowed inside the wrapper). - Runtime selection.
getServerRuntimereadshasCloudflarePluginlazily; it's set in theconfighook, which runs beforeconfigEnvironment(optimizer registration) and well before any transform/renderChunk, so Worker→Edge vs Node→chunk identity is always resolved by the time it's read. - Optimizer exclude broadening is coherent.
react/react-dom/react-dom/serverare already in this Pages SSR env'sresolve.external, so excluding them from the dep optimizer matches the existingSSR_EXTERNAL_REACT_ENTRIESrationale (avoid duplicate React records →React.Hnull). AddingObject.keys(nextShimMap)is consistent with those being alias-resolved.
Minor / non-blocking
-
E2E host inconsistency.
tests/e2e/pages-router-prod/cjs-dependency-globals.browser.spec.tsuseshttp://127.0.0.1:4175while every sibling spec (production.spec.ts,interactive.spec.ts,gssp-data-dedup.spec.ts,javascript-urls.spec.ts) useshttp://localhost:4175. Functionally equivalent; worth aligning. ThePAGES_ROUTER_PROD_BASE_URLenv fallback is otherwise unused by siblings. -
commonJsPlugin.transformreassignment assumes the function form. The wrapper is guarded bytypeof commonJsTransform === "function". vite-plugin-commonjs 0.10.4 exposestransformas a plain function, so this works today, but if it ever migrates to the Vite 8 object form ({ filter, handler }) the wrapper silently no-ops and environment-awareness is lost (bundled deps would stop being transformed in dev). A latent fragility, not a bug — consider a comment or a defensive assertion pinning the expected shape. -
Test fixture couples to a Next.js internal. The fixtures import
next/dist/compiled/regenerator-runtime/path, which happens to read__dirname. Convenient and real, but tied to Next's compiled layout; if Next drops/relocates that file the tests break for an unrelated reason. Acceptable given it's a genuine bundled-CJS-reads-__dirnamecase, just flagging the coupling.
No correctness issues found
Dev/prod parity holds, the relocatability contract is tested against real Nitro and workerd runtimes, and the Node/Edge identity split matches Next.js Pages-bundle behavior. LGTM.
|
To https://github.com/cloudflare/vinext.git |
b17f08c to
0338ffe
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review — PR #2826 (fix(pages): preserve CJS globals in bundled dependencies)
Traced the dev/build/optimizer code paths, verified the vite-plugin-commonjs@0.10.4 internals the wrapper depends on, checked runtime selection timing, and ran the touched suites.
Verdict: LGTM — no blocking correctness issues found. A few observations below, none blocking.
What I verified
tests/import-meta-url.test.ts— 55 passedtests/pages-router.test.ts -t "bundled"— 3 passed (real dev route + relocated-prod build assertingimport.meta.dirnamein the server chunk and.not.toContain("/node_modules/"))vp checkonimport-meta-url.ts+index.ts— clean
Design is correct
- The sync-filter bridge is sound. Confirmed against
vite-plugin-commonjs@0.10.4:transformreturnstransformCommonjs(...), which runs synchronously throughoptions.filter?.call(id)(index.mjs L408) before the firstawait(dynaimcRequire.generateRuntime, L422). The wrapper'stry { return transform(...) } finally { reset }resets the flag when the promise is returned (synchronously), which is after the filter has already been consulted. SotransformBundledCommonJsDependenciesis guaranteed set when the filter reads it, and concurrent transforms cannot cross-talk. The "do not await here" comment is accurate. vite-plugin-commonjs's own optimizer path doesn't apply under Vite 8. It pushes anoptimizeDeps.esbuildOptionspre-bundle plugin (index.mjs L356-358), but this repo resolvesviteto Vite 8 / Rolldown (optimizeDeps.rolldownOptions), so that esbuild hook is inert. Registering the separateoptimizeDepsPluginviarolldownOptions.pluginsinconfigEnvironmentis the right way to cover the Rolldown pre-bundle pipeline.- Runtime selection timing is safe.
getServerRuntimereadshasCloudflarePlugin, which is assigned inside theconfighook body (index.ts L2650) — this completes beforeconfigEnvironment(optimizer registration) and well before anytransform/renderChunk. So Worker→Edge vs Node→chunk identity is always resolved by read time. - Idempotency / no double-injection.
analyzeServerCjsGlobalsrecords module-scope bindings and skips injection when__dirname/__filenameis already bound. This correctly preventsrenderChunkfrom redeclaring a global already emitted by the optimizer rewrite or by rolldown's CJS interop (verified by thenot.toContain("var __dirname = void 0")assertion incjs-globals-runtime.test.ts). - Node/Nitro/Worker runtime split matches Next.js. Chunk-relative
import.meta.dirnamefor Node/Nitro, Edge-logical/index.js///for Workers — each proved against real Nitro and workerd runtimes incjs-globals-runtime.test.ts.
Observations (non-blocking)
-
Project-source
__dirnameis not relocatable in Node/Nitro production — pre-existing, not introduced here. For project source (non-node_modules), the build transform bakes the absolute build-host source path (cjs-globals-runtime.test.tsL269-273 / L415-416 assertproject-runtime-path=canonicalRoot/pages/..., i.e. the original source root, even in relocated Nitro output). Only bundled-dependency__dirnamegets the relocatable chunk-relative treatment viarenderChunk. This asymmetry is confirmed to exist onmainalready (norenderChunkthere; project source was baked in both dev and build), so this PR does not regress it — but the PR body's "relocatable emitted-chunk identity in Node/Nitro production" claim only holds for dependencies, not project source. Worth a follow-up: Next's Node-target webpack makes project-source__dirnamethe emitted-chunk directory too, so this is a latent parity gap. -
Chunk-wide binding guard can under-inject in a rare mixed chunk.
analyzeServerCjsGlobalsruns over the whole emitted chunk. If module A in a chunk already has a top-levelvar __dirname(e.g. optimizer-injected) and module B in the same chunk reads__dirnamefreely, the chunk-widemoduleBindingsguard skipsrenderChunkinjection entirely, and B's read resolves to A's chunk-scoped value. Requires two bundled CJS deps reading__dirnamein one chunk with divergent expected values — very rare, and no test exercises it, but it's a real edge of the chunk-level (vs module-level) analysis. -
commonJsPlugin.transformreassignment assumes the function form. Guarded bytypeof commonJsTransform === "function". Works today (0.10.4 exposes a plain function), but if it ever migrates to the Vite 8 object form ({ filter, handler }) the wrapper silently no-ops and bundled deps stop being environment-scoped in dev. Consider a defensive assertion or comment pinning the expected shape. -
E2E host inconsistency.
tests/e2e/pages-router-prod/cjs-dependency-globals.browser.spec.tsuseshttp://127.0.0.1:4175while every sibling spec (production.spec.ts,interactive.spec.ts,gssp-data-dedup.spec.ts,javascript-urls.spec.ts) useshttp://localhost:4175. Functionally equivalent; worth aligning. ThePAGES_ROUTER_PROD_BASE_URLenv fallback is otherwise unused by siblings. -
mergeOptimizeDepsExcludebroadening is the highest-surface change. SSRexcludegrew from["ipaddr.js"]to addreact,react-dom,react-dom/server, and allnextShimMapkeys. Coherent — those React entries already sit in this env'sresolve.external, andnextShimMapkeys are alias-resolved — but it's a broader optimizer behavior change than the CJS-globals fix strictly needs. Fine as-is; flagging as the biggest behavioral surface.
Bottom line
Dev/prod parity holds for dependencies, the relocatability contract is tested against real Nitro and workerd runtimes, and the Node/Edge identity split matches Next.js Pages-bundle behavior. The concurrency reasoning in the commonjs wrapper is correct against the pinned plugin version. Non-blocking notes above.
|
Review posted to PR #2826. SummaryI reviewed PR #2826 ( Verdict: LGTM — no blocking correctness issues found. Key things I confirmed as correct:
Five non-blocking observations flagged:
|
Co-authored-by: james-elicx <james-elicx@users.noreply.github.com>
841c9a1 to
0338ffe
Compare
0338ffe to
f12d38d
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review — PR #2826 (fix(pages): preserve CJS globals in bundled dependencies)
Independent trace of the dev/build/optimizer paths, verified vite-plugin-commonjs@0.10.4 internals against the pinned source, checked runtime-selection timing, and ran the touched suites (tests/import-meta-url.test.ts 55 passed; tests/pages-router.test.ts -t "bundled" 3 passed).
Verdict: LGTM — no blocking correctness issues found. The design is sound and the runtime coverage (real Nitro + workerd + relocated Node prod) is exactly what this class of fix needs. Findings below are non-blocking; several corroborate the earlier reviews.
Verified correct
- Sync-filter concurrency bridge is safe. Confirmed against
dist/index.mjs:transform(code, id)returnstransformCommonjs(...), which invokesoptions.filter?.call(options, id)(L408) synchronously before the firstawait(generateRuntime, L422). The wrapper'stry { return … } finally { reset }resets the flag when the promise is returned (synchronous, noawaitin the wrapper), so the flag is guaranteed live when the filter reads it and concurrent transforms cannot cross-talk. The "do not await here" comment is accurate. The early!isCommonjs(code)bail (L407) runs before the filter, so non-CJS inputs never consult it — fine. - Optimizer split is justified.
commonjs()pushes anoptimizeDeps.esbuildOptionsprebundle plugin; this repo is Vite 8 / Rolldown (rolldownOptions), so that hook is inert. RegisteringoptimizeDepsPluginviarolldownOptions.pluginsinconfigEnvironmentis the correct way to cover the Rolldown prebundle. - Runtime-selection timing.
getServerRuntimereadshasCloudflarePlugin, set in theconfighook (index.ts:2650), which completes beforeconfigEnvironmentand anytransform/renderChunk. Worker→Edge vs Node→chunk identity is always resolved by read time. - Idempotency.
analyzeServerCjsGlobalsskips injection when a global is already module-bound, preventingrenderChunkfrom redeclaring a global emitted by the optimizer rewrite or rolldown's CJS interop (asserted bynot.toContain("var __dirname = void 0")). - Node/Nitro/Worker split matches Next.js and is proven end-to-end against real runtimes.
Non-blocking
-
Chunk-wide binding guard can under-inject in a rare mixed chunk.
analyzeServerCjsGlobalsruns over the whole emitted chunk. If module A already has a top-levelvar __dirnameand module B in the same chunk reads a free__dirname, the chunk-widemoduleBindingsguard suppressesrenderChunkinjection and B resolves to A's value. Requires two bundled CJS deps reading__dirnamein one chunk — very rare, no test exercises it, but it's a real edge of chunk-level (vs module-level) analysis. Consider a follow-up test if this becomes a concern. -
commonJsPlugin.transformreassignment assumes the function form. Guarded bytypeof commonJsTransform === "function". Works on 0.10.4 (plain function), but if the plugin ever migrates to the Vite 8 object hook form ({ filter, handler }) the wrapper silently no-ops and bundled deps stop being environment-scoped in dev — no error, just a behavioral regression. Worth a defensive assertion or a comment pinning the expected shape so a dependency bump surfaces it loudly. -
Project-source
__dirnameis not relocatable in Node/Nitro production. Only bundled dependency__dirnamegets the chunk-relativeimport.meta.dirnametreatment viarenderChunk; project source still bakes the absolute build-host path (cjs-globals-runtime.test.ts:262-273assertproject-runtime-path= the original source root even in relocated Nitro output). This is pre-existing onmain(norenderChunkthere), so the PR does not regress it, but the PR body's "relocatable emitted-chunk identity in Node/Nitro production" claim holds only for dependencies, not project source. Next's Node-target webpack makes project-source__dirnamechunk-relative too — a latent parity gap worth a follow-up. -
E2E host inconsistency.
tests/e2e/pages-router-prod/cjs-dependency-globals.browser.spec.ts:3useshttp://127.0.0.1:4175while every sibling (production.spec.ts,interactive.spec.ts,gssp-data-dedup.spec.ts,javascript-urls.spec.ts) useshttp://localhost:4175. Functionally equivalent; align for consistency. ThePAGES_ROUTER_PROD_BASE_URLfallback is otherwise unused by siblings. -
mergeOptimizeDepsExcludebroadening is the highest-surface change. The SSRexcludegrew from["ipaddr.js"]to addreact,react-dom,react-dom/server, and allnextShimMapkeys. Coherent — those React entries already sit in this env'sresolve.external, andnextShimMapkeys are alias-resolved — and the newtests/pages-router.test.tstest locks the contract. Fine as-is; flagging as the biggest behavioral surface beyond the strict CJS-globals fix.
Bottom line
Dev/prod parity holds for dependencies, the relocatability contract is tested against real Nitro and workerd, and the Node/Edge identity split matches Next.js Pages-bundle behavior. The concurrency reasoning in the commonjs wrapper is correct against the pinned plugin version. Ship it; the two items I'd most want addressed before/after merge are #2 (defensive shape assertion) and #4 (host alignment).
|
Review posted to PR #2826. SummaryI independently reviewed PR #2826 ( Verdict: LGTM — no blocking correctness issues found. Things I verified as correct:
Five non-blocking findings flagged:
My findings corroborate the earlier reviews. The two items I'd most want addressed are #2 and #4. |
1 participant
Summary
__dirname/__filenamehandling into the existing module-identity capabilityRegression coverage
nitro/vitebuild executed through.output/server/index.mjsUpstream behaviour
Next.js bundles dependencies per module layer; the presence of an App Router does not disable Pages dependency bundling. Node-target webpack exposes emitted-chunk identity. Next's Edge webpack/Rspack configuration uses logical
/index.jsand/values because no filesystem module identity exists.Relevant Next.js sources:
packages/next/src/build/handle-externals.tspackages/next/src/build/webpack/config/blocks/base.tspackages/next/src/build/webpack-config.tstest/e2e/externals-pages-bundle/externals-pages-bundle.test.tsValidation
vp test run tests/import-meta-url.test.ts tests/build-optimization.test.ts— 203 passed, 2 skippedvp test run tests/cjs-globals-runtime.test.ts— real Nitro and workerd runtimes passedvp checkandgit diff --check— clean