fix(config): share CommonJS module fallback

[james-elicx]

Summary

• extract the CommonJS .js fallback introduced in fix(css): load local CommonJS PostCSS plugins #1961 into a shared Node-only loader
• use it for PostCSS config files, PostCSS string plugins, and legacy next.config.js loading
• remove temporary sibling .cjs files from the next-config fallback
• preserve recursive and deferred CommonJS require() behavior across .js and .cjs boundaries

Compatibility

• preserves Node module cache, module.parent, circular dependencies, JSON, builtins, and native addons
• supports synchronous and asynchronous exported factories that call require() after initial module evaluation
• avoids retrying syntax errors or user-thrown errors that mimic CommonJS loader failures
• supports read-only and symlinked package targets without writing temporary files

Tests

• vp check packages/vinext/src/utils/commonjs-loader.ts packages/vinext/src/plugins/postcss.ts packages/vinext/src/config/next-config.ts tests/commonjs-loader.test.ts tests/postcss-resolve.test.ts tests/next-config.test.ts
• vp test run tests/commonjs-loader.test.ts tests/postcss-resolve.test.ts tests/next-config.test.ts (211 passed)
• vp run vinext#build

Upstream reference

Next.js uses Module._compile plus a .js require hook for config loading:

• https://github.com/vercel/next.js/blob/canary/packages/next/src/build/next-config-ts/transpile-config.ts
• https://github.com/vercel/next.js/blob/canary/packages/next/src/build/next-config-ts/require-hook.ts

[james-elicx]

/bigbonk review for issues

[pkg-pr-new]

Open in StackBlitz

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

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

commit: d7c8f2c

[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 — fix(config): share CommonJS module fallback

Solid refactor. I traced the new shared commonjs-loader.ts against the inline implementation it replaces (#1961) and exercised the loader directly across the tricky paths: nested misclassified .js, .js↔.cjs boundaries, circular deps, native require() of an ESM-typed .js config, deferred require() inside exported (async) factories, and node:/JSON/native-addon delegation. All behave correctly, and vp check + the three targeted test files (commonjs-loader, next-config, postcss-resolve) pass. No blocking issues found.

A few low-priority observations, none blocking:

1. shouldRetryAsCommonJs only inspects the first .js stack frame

packages/vinext/src/utils/commonjs-loader.ts:43-60 — the loop returns on the first frame whose path ends in .js, comparing the source slice at the reported column against the identifier. If the topmost .js frame isn't the one that referenced the missing CJS global (e.g. an inlined/transpiled helper, or a frame whose on-disk source no longer matches the executed column), this returns a false negative and the retry is skipped. For the config/postcss flows the file is read straight from disk so columns line up, but the heuristic is inherently fragile. Consider scanning all .js frames before giving up rather than bailing on the first match.

2. Source-line/column matching assumes untransformed on-disk source

Same function — fs.readFileSync(canonicalFramePath) then slice(column, column+identifier.length). This is only reliable when the executed module is byte-identical to what's on disk. It is today (these loaders run on raw config/plugin files), but it's worth a comment noting the assumption so a future caller doesn't pass transformed/bundled inputs and silently lose the fallback.

3. .cjs dependencies are unconditionally routed through the custom compiler

commonjs-loader.ts:85-87 — any .cjs resolved from a compiled module is recompiled via compileCommonJsModule rather than delegated to originalRequire. This is the intended cache-coherence behavior (and tests cover the .cjs boundary), but it does mean .cjs files bypass Node's own loader extensions/require.extensions. Fine for the current use cases; flagging in case a future .cjs plugin relies on a custom extension hook.

Things I explicitly verified are NOT problems

• canonicalPath /@fs/ slice(4) (line 15): correct for posix — Vite emits /@fs/etc/passwd (marker /@fs + absolute path), so slice(4) yields /etc/passwd. (Would be wrong on Windows drive paths, but that's a pre-existing platform gap and not exercised here.)
• require() of an ESM-only package from a compiled .cjs: on Node 22+ require(esm) succeeds natively so the fallback is never hit; on older Node the _compile attempt would throw a SyntaxError (correct failure, not a silent mis-load).
• Global require.cache reuse instead of fix(css): load local CommonJS PostCSS plugins #1961's per-call Map: consistent with the existing postcssCache result memoization; not a regression.
• mod.default ?? mod unwrap in importExportWithCommonJsFallback: matches the prior loadPluginExport behavior; CJS-with-default-property is covered by a test.

[ask-bonk]

Review posted to PR #1990.

Summary

I reviewed the CommonJS loader extraction in PR #1990 by tracing the new shared commonjs-loader.ts against the inline #1961 implementation it replaces, and empirically exercising the loader across the tricky code paths (nested misclassified .js, .js↔.cjs boundaries, circular deps, native require() of an ESM-typed config, deferred sync/async factory require(), builtin/JSON/native-addon delegation). I also ran vp check and the three targeted test files — all pass.

No blocking issues. The refactor faithfully preserves the prior behavior while sharing the loader, and the test coverage is thorough.

Three low-priority observations, all non-blocking:

1. shouldRetryAsCommonJs bails on the first .js stack frame — could be a false negative if the topmost .js frame isn't the one referencing the missing CJS global.
2. The source-line/column matching assumes untransformed on-disk source — true today, worth a comment.
3. .cjs deps are unconditionally recompiled through the custom loader, bypassing Node's require.extensions (intended for cache coherence).

I also documented four things I verified are not problems (the /@fs/ slice(4), require(esm) on Node 22+, global require.cache reuse, and the mod.default ?? mod unwrap), so reviewers don't re-flag them.

github run

[james-elicx]

Addressed the review observations in d7c8f2c: retry detection now scans all candidate .js frames, and the raw-source plus .cjs loader assumptions are documented. Targeted checks remain green (211 tests).

[james-elicx]

/bigbonk review for correctness issues

[ask-bonk]

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

[james-elicx]

/bigbonk review for correctness issues

[ask-bonk]

Review — fix(config): share CommonJS module fallback

I traced the new shared commonjs-loader.ts against the inline #1961 implementations it replaces, then exercised the loader end-to-end on Node 22 legacy semantics (--no-experimental-require-module), which is the actual scenario this fallback exists for. On modern Node (22.12+/24) require(esm) and import() of CJS-in-type:module both succeed transparently, so the fallback is dormant — the legacy run is what actually validates correctness.

Verdict: correct, no blocking issues. Below is what I verified plus one pre-existing behavior worth flagging.

Empirically verified correct (under legacy Node require/ESM semantics)

• next.config.js entry fallback — require() throws ERR_REQUIRE_ESM; the message contains the realpath, so the error.message.includes(canonicalResolvedPath) check (commonjs-loader.ts:31) matches and the retry fires. Loaded the correct config object.
• Symlinked / read-only package targets — ERR_REQUIRE_ESM messages reference the realpath even for symlinked entries, so the canonicalPath comparison holds. No temp files written (matches the new next-config.test.ts assertions).
• Nested .js → .js requires and circular deps resolve correctly via the mod.require override + global require.cache.
• require.resolve / require.cache inside the compiled module still work — Node's _compile wraps our overridden mod.require with makeRequireFunction, so .resolve/.cache are attached to the in-module require while load delegation still routes through the override. Confirmed both work from a fallback-loaded config.
• .cjs boundary, deferred (sync + async) factory require(), builtin/JSON/native-addon delegation — all behave per the new tests.
• Stack-frame regex (commonjs-loader.ts:47) — greedy .+ correctly captures file:// URL frames (extra colons), and node:internal/... frames are filtered out by the .endsWith(".js") guard. The d7c8f2c change to scan all .js frames removes the first-frame false-negative risk the prior review raised.
• Column matching — V8's 1-based columns map to slice(column-1, ...) correctly for both line-start and mid-line identifiers (verified with indented/prefixed source).
• User-thrown spoof errors (ReferenceError/ERR_REQUIRE_ESM) are not retried — the column check on the throw line doesn't match the identifier, and the side-effect counter tests confirm no recompile.

Non-blocking: side effects run twice on the import→CJS fallback (pre-existing)

importExportWithCommonJsFallback (commonjs-loader.ts:128) does await import(...) then, on an ESM-scope failure, re-runs the file from scratch via loadCommonJsModule. I confirmed the import() attempt executes top-level side effects before throwing the ReferenceError, so a config/plugin with non-idempotent top-level side effects runs them twice (counter = 2 in a direct repro).

This is inherited unchanged from #1961 (same try-import / catch-recompile shape), not a regression in this PR, and config/plugin top-levels are normally idempotent. Flagging only so it's tracked — a future improvement could be to attempt the cheap shouldRetryAsCommonJs classification without re-importing, or to gate the eager import() for .js-in-type:module files. Not required for this PR.

Tests / checks

• vp check on the three changed source files: clean (format, lint, types).
• vp test run tests/commonjs-loader.test.ts tests/next-config.test.ts tests/postcss-resolve.test.ts: all green (195 + 16).

Nice extraction — the shared loader is a faithful, well-tested consolidation of the two prior inline copies, and the symlink/read-only and .cjs-boundary coverage is a real improvement over #1961.

[ask-bonk]

Review posted to PR #1990.

Summary

I reviewed the CommonJS loader extraction for correctness by tracing the new shared commonjs-loader.ts against the two inline implementations it replaces (#1961's postcss.ts loader and next-config.ts's temp-.cjs copy), then empirically exercising the fallback under legacy Node semantics (--no-experimental-require-module) — which is the only mode where this code path actually runs, since modern Node (22.12+/24) loads CJS-in-type:module transparently.

No blocking correctness issues. The refactor faithfully preserves CJS semantics and the legacy fallback works end-to-end.

What I verified empirically:

• ERR_REQUIRE_ESM messages contain the realpath, so the canonicalResolvedPath comparison matches — including for symlinked/read-only package targets (the new test scenario).
• Nested .js requires, circular deps, .cjs boundaries, deferred sync/async factories all resolve correctly.
• require.resolve / require.cache still work inside fallback-compiled modules (Node's _compile wraps the overridden mod.require).
• The d7c8f2c hardening (scan all .js frames) correctly removes the first-frame false-negative.
• User-thrown spoof errors are not retried.

One non-blocking, pre-existing observation (inherited unchanged from #1961, not introduced here): importExportWithCommonJsFallback re-runs the file from scratch after a failed import(), and I confirmed the import() attempt executes top-level side effects before throwing — so non-idempotent config/plugin top-levels run twice. Flagged for tracking only.

vp check and the three targeted test files all pass.

github run