Skip to content

fix: include package modules in commonjs transform #218

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.

Sign up for GitHub

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

Open
zebp wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: include package modules in commonjs transform #218

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/vinext/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1668,7 +1668,16 @@ hydrate();
tsconfigPaths(),
// Transform CJS require()/module.exports to ESM before other plugins
// analyze imports (RSC directive scanning, shim resolution, etc.)
commonjs(),
commonjs({
// vite-plugin-commonjs excludes node_modules by default, but there's
// nothing preventing a dependency from using CJS, so we need to include
// node_modules here.
filter(id) {
if (getPackageName(id) !== null) {
return true
}
},
Comment on lines +1675 to +1679
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: The implicit undefined return for non-node_modules files is correct (it defers to the plugin's default behavior, which includes them), but it reads like an accidental omission. A brief comment or explicit return undefined would make the intent clearer.

Also, this unconditionally returns true for all node_modules files. That's a broader scope than the problem description suggests — it will now attempt CJS-to-ESM transforms on every file in every dependency, not just the ones that actually use CJS. The plugin does have a regex pre-check for require/module/exports tokens before actually transforming, so it shouldn't break anything, but it does add unnecessary work to the transform pipeline for ESM-only deps.

If you've measured and this doesn't noticeably affect dev startup time, this is fine as-is. Otherwise, you could scope it down to only return true when the file extension is .cjs or when the dependency's package.json doesn't have "type": "module" — but that's probably over-engineering for now.

Suggested change
filter(id) {
if (getPackageName(id) !== null) {
return true
}
},
filter(id) {
// vite-plugin-commonjs excludes node_modules by default, but there's
// nothing preventing a dependency from using CJS, so we need to include
// node_modules here. Returning undefined for non-node_modules files
// preserves the default behavior (include them).
if (getPackageName(id) !== null) {
return true
}
},

}),
{
name: "vinext:config",
enforce: "pre",
Expand Down
Loading