fix: include package modules in commonjs transform

[zebp]

Previously we only transformed CJS modules if they were part of the part of the project and not if they were in the project's dependencies. Now we will apply CJS to ESM transformations if the vite module id can be resolved to a node module.

[pkg-pr-new]

Open in StackBlitz

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

commit: 6b23929

[southpolesteve]

/bigbonk review this. Also figure out why tests are failing

[ask-bonk]

Good fix — vite-plugin-commonjs excludes node_modules by default, which silently breaks any dependency that ships CJS. The three-way filter semantics (true = force include, false = force exclude, undefined = default behavior) are used correctly here.

One issue: the filter currently returns true for node_modules but undefined for project files, which still works correctly (undefined defers to the default include-non-node_modules behavior), but the implicit undefined return is worth a note — see inline comment.

[ask-bonk]

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
	}
	},

[ask-bonk]

Review posted. Here's a summary of my findings:

The fix is correct. vite-plugin-commonjs excludes node_modules by default, so any dependency shipping CJS (which is still common) would silently fail to get transformed. The filter callback correctly uses the three-way return semantics: true to force-include node_modules files, undefined (implicit) to defer to default behavior for project files.

One minor note I left inline: the filter opts all node_modules files into the CJS transform pipeline, not just the ones that actually use CJS. The plugin has a pre-check regex that avoids transforming files without require/module/exports tokens, so this won't cause correctness issues, but it does add some overhead. If dev startup time isn't noticeably affected, it's fine as-is. I also suggested adding a brief comment clarifying the implicit undefined return for non-node_modules files.

github run