fix: support CommonJS node_modules in Pages Router dev#665
fix: support CommonJS node_modules in Pages Router dev#665southpolesteve wants to merge 1 commit intomainfrom
Conversation
commit: |
|
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good approach overall. The noExternal: true config that vinext sets for non-Cloudflare environments means all node_modules are forced through Vite's transform pipeline. Since vite-plugin-commonjs skips node_modules by default, and ssrTransform only handles ESM syntax (not require()/module.exports), CJS patterns survive into runInlinedModule where ESModulesEvaluator can't handle them. The evaluator-level fix is the right layer for this.
The scoping to node_modules + .cjs is appropriately conservative. The test creates a realistic scenario with inter-file require() between CJS modules. The replaceNamespaceExports/toCommonJsNamespace conversion correctly handles the ESM namespace shape that the module runner expects.
One real bug (Windows path separators) needs fixing before merge. A couple minor items noted inline.
| ESModulesEvaluator, | ||
| createNodeImportMeta, | ||
| ssrDynamicImportKey, | ||
| ssrExportAllKey, |
There was a problem hiding this comment.
Bug: path.sep is \\ on Windows, but Vite normalizes all module paths to forward slashes via normalizePath() (which calls path.posix.normalize(slash(id))). So mod.file will always contain forward slashes like C:/Users/dev/project/node_modules/foo/index.js.
This means on Windows the check becomes modulePath.includes("\\node_modules\\") which will never match — the entire CJS compat layer silently becomes a no-op.
| ssrExportAllKey, | |
| (modulePath.includes("/node_modules/") || modulePath.endsWith(".cjs")) |
| const COMMONJS_MARKERS = | ||
| /\bmodule\.exports\b|\bexports(?:\s*\[|\.[A-Za-z_$])|\brequire\s*\(|\b__dirname\b|\b__filename\b/; | ||
| const AsyncFunction = async function () {}.constructor as new ( | ||
| ...args: string[] |
There was a problem hiding this comment.
Nit: Worth a brief inline comment that createRequire loads files from disk directly, bypassing Vite's module graph. This is fine for node_modules (no HMR needed, no user plugins relevant), but the trade-off is worth documenting since require("./core.js") inside CJS packages will resolve sub-modules natively rather than through Vite's transform pipeline.
Something like:
// createRequire resolves sub-modules natively (bypasses Vite's module graph).
// This is intentional for node_modules — third-party deps don't need HMR
// or Vite plugin transforms.
const require = createRequire(pathToFileURL(modulePath));| */ | ||
|
|
||
| import { ModuleRunner, ESModulesEvaluator, createNodeImportMeta } from "vite/module-runner"; | ||
| import path from "node:path"; |
There was a problem hiding this comment.
Minor: This regex tests the raw code string, so it can false-positive on CJS markers in string literals or comments (e.g., a JSDoc mentioning require()). Combined with the node_modules path gate, the practical risk is very low — the worst case is adding unused CJS params to an already-ESM function, which is harmless. Not blocking, just noting.
|
The review has been posted. Here's a summary of what I found and commented: Review Summary for PR #665Verdict: Request Changes (one bug, two minor items) Bug: Windows path separator (must fix)
Minor:
|
1 participant
Summary
node_modulesand.cjsmodules, while preserving Vite's normal ESM evaluator everywhere elsenode_modulesThis matches the Pages Router bug reported in #585. In Next.js, Pages Router dependencies are externalized by default unless
bundlePagesRouterDependenciesis enabled, so raw CommonJS packages should not crash dev SSR withmodule is not defined.Fixes #585.
Verification
vp test run tests/cjs.test.ts tests/node-modules-css.test.tsvp check packages/vinext/src/server/dev-module-runner.ts tests/cjs.test.tsNotes
@vitejs/plugin-rscpipeline and needs separate work tracked in App Router dev still fails on raw CommonJS packages from node_modules #666.