-
Notifications
You must be signed in to change notification settings - Fork 275
fix: support CommonJS node_modules in Pages Router dev #665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -56,7 +56,21 @@ | |||||
| * ``` | ||||||
| */ | ||||||
|
|
||||||
| import { ModuleRunner, ESModulesEvaluator, createNodeImportMeta } from "vite/module-runner"; | ||||||
| import path from "node:path"; | ||||||
| import { createRequire } from "node:module"; | ||||||
| import { pathToFileURL } from "node:url"; | ||||||
| import { | ||||||
| ModuleRunner, | ||||||
| ESModulesEvaluator, | ||||||
| createNodeImportMeta, | ||||||
| ssrDynamicImportKey, | ||||||
| ssrExportAllKey, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: This means on Windows the check becomes
Suggested change
|
||||||
| ssrExportNameKey, | ||||||
| ssrImportKey, | ||||||
| ssrImportMetaKey, | ||||||
| ssrModuleExportsKey, | ||||||
| } from "vite/module-runner"; | ||||||
| import type { EvaluatedModuleNode, ModuleEvaluator, ModuleRunnerContext } from "vite/module-runner"; | ||||||
| import type { DevEnvironment } from "vite"; | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -73,6 +87,121 @@ export interface DevEnvironmentLike { | |||||
| ) => Promise<Record<string, unknown>>; | ||||||
| } | ||||||
|
|
||||||
| 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[] | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Worth a brief inline comment that 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)); |
||||||
| ) => (...args: unknown[]) => Promise<unknown>; | ||||||
|
|
||||||
| function shouldUseCommonJsCompat(modulePath: string, code: string): boolean { | ||||||
| return ( | ||||||
| COMMONJS_MARKERS.test(code) && | ||||||
| (modulePath.includes(`${path.sep}node_modules${path.sep}`) || modulePath.endsWith(".cjs")) | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| function replaceNamespaceExports( | ||||||
| target: Record<PropertyKey, unknown>, | ||||||
| source: Record<PropertyKey, unknown>, | ||||||
| ): void { | ||||||
| for (const key of Reflect.ownKeys(target)) { | ||||||
| const descriptor = Object.getOwnPropertyDescriptor(target, key); | ||||||
| if (descriptor?.configurable) { | ||||||
| delete target[key]; | ||||||
| } | ||||||
| } | ||||||
| for (const key of Reflect.ownKeys(source)) { | ||||||
| const existing = Object.getOwnPropertyDescriptor(target, key); | ||||||
| if (existing && !existing.configurable) continue; | ||||||
| const descriptor = Object.getOwnPropertyDescriptor(source, key); | ||||||
| if (descriptor) { | ||||||
| Object.defineProperty(target, key, descriptor); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| function toCommonJsNamespace(value: unknown): Record<PropertyKey, unknown> { | ||||||
| const namespace = Object.create(null) as Record<PropertyKey, unknown>; | ||||||
| Object.defineProperty(namespace, Symbol.toStringTag, { | ||||||
| value: "Module", | ||||||
| enumerable: false, | ||||||
| configurable: false, | ||||||
| }); | ||||||
| Object.defineProperty(namespace, "default", { | ||||||
| enumerable: true, | ||||||
| configurable: true, | ||||||
| value, | ||||||
| }); | ||||||
|
|
||||||
| if (value && (typeof value === "object" || typeof value === "function")) { | ||||||
| for (const key of Object.keys(value)) { | ||||||
| if (key === "default" || key === "__esModule") continue; | ||||||
| Object.defineProperty(namespace, key, { | ||||||
| enumerable: true, | ||||||
| configurable: true, | ||||||
| get: () => value[key as keyof typeof value], | ||||||
| }); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return namespace; | ||||||
| } | ||||||
|
|
||||||
| class CommonJsCompatEvaluator implements ModuleEvaluator { | ||||||
| private readonly fallback = new ESModulesEvaluator(); | ||||||
| readonly startOffset = this.fallback.startOffset; | ||||||
|
|
||||||
| async runInlinedModule( | ||||||
| context: ModuleRunnerContext, | ||||||
| code: string, | ||||||
| mod: Readonly<EvaluatedModuleNode>, | ||||||
| ): Promise<void> { | ||||||
| const modulePath = mod.file; | ||||||
| if (!modulePath || !path.isAbsolute(modulePath) || !shouldUseCommonJsCompat(modulePath, code)) { | ||||||
| await this.fallback.runInlinedModule(context, code); | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| const exportsObject = context[ssrModuleExportsKey] as Record<PropertyKey, unknown>; | ||||||
| const module = { exports: exportsObject as unknown }; | ||||||
| const require = createRequire(pathToFileURL(modulePath)); | ||||||
|
|
||||||
| await new AsyncFunction( | ||||||
| ssrModuleExportsKey, | ||||||
| ssrImportMetaKey, | ||||||
| ssrImportKey, | ||||||
| ssrDynamicImportKey, | ||||||
| ssrExportAllKey, | ||||||
| ssrExportNameKey, | ||||||
| "module", | ||||||
| "exports", | ||||||
| "require", | ||||||
| "__filename", | ||||||
| "__dirname", | ||||||
| `"use strict";${code}`, | ||||||
| )( | ||||||
| context[ssrModuleExportsKey], | ||||||
| context[ssrImportMetaKey], | ||||||
| context[ssrImportKey], | ||||||
| context[ssrDynamicImportKey], | ||||||
| context[ssrExportAllKey], | ||||||
| context[ssrExportNameKey], | ||||||
| module, | ||||||
| module.exports, | ||||||
| require, | ||||||
| modulePath, | ||||||
| path.dirname(modulePath), | ||||||
| ); | ||||||
|
|
||||||
| replaceNamespaceExports(exportsObject, toCommonJsNamespace(module.exports)); | ||||||
| Object.seal(exportsObject); | ||||||
| } | ||||||
|
|
||||||
| runExternalModule(filepath: string): Promise<unknown> { | ||||||
| return this.fallback.runExternalModule(filepath); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Build a ModuleRunner that calls `environment.fetchModule()` directly, | ||||||
| * bypassing the hot channel entirely. | ||||||
|
|
@@ -126,6 +255,6 @@ export function createDirectRunner(environment: DevEnvironmentLike | DevEnvironm | |||||
| sourcemapInterceptor: false, | ||||||
| hmr: false, | ||||||
| }, | ||||||
| new ESModulesEvaluator(), | ||||||
| new CommonJsCompatEvaluator(), | ||||||
| ); | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thenode_modulespath 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.