fix(cli): ensure type:module before Vite loads vite.config.ts (fixes #184)

[yunus25jmi1]

Problem

Projects without "type": "module" in package.json cause Vite to bundle vite.config.ts through esbuild's CJS path, producing a .vite-temp/*.timestamp.mjs file that contains require() calls for the imported plugins. On Node 22 in CI (GitHub Actions, ubuntu-latest), require()-ing a .mjs file is not supported and the build fails immediately:

failed to load config from /home/runner/work/.../vite.config.ts
Error: Dynamic require of "file:///…/@cloudflare/vite-plugin/dist/index.mjs" is not supported
    at file:///.../node_modules/.vite-temp/vite.config.ts.timestamp-xxx.mjs:5:9

The affected user had set up their project manually (without vinext init) or before the init step added "type": "module". vinext init already adds this field, but vinext dev / vinext build were not enforcing it.

Fix

Call ensureESModule(root) + renameCJSConfigs(root) in both the dev() and buildApp() CLI commands, before Vite is loaded. This reuses the same logic already used by vinext init.

• renameCJSConfigs renames postcss.config.js / tailwind.config.js / .eslintrc.js etc. → .cjs first, preventing breakage in those files once "type": "module" is added
• ensureESModule adds "type": "module" to package.json
• Both are no-ops when the project is already correctly configured (no extra I/O on the hot path)
• A warning is printed when a change is made, pointing users to run vinext init to make it permanent

Tests

9 new unit tests added to tests/deploy.test.ts under the describe block "ensureESModule + renameCJSConfigs — issue #184 compatibility":

• Adds type:module when missing
• Idempotent — safe to call multiple times
• Returns false when no package.json exists
• Renames CJS postcss.config.js → postcss.config.cjs before adding type:module
• Skips rename for ESM-style postcss.config.js (no module.exports or require())
• Renames tailwind.config.js when CJS
• Renames .eslintrc.js when CJS
• Renames CJS config that uses require() (not only module.exports)
• Handles multiple CJS config files simultaneously

All 169 tests pass (pnpm exec vitest run tests/deploy.test.ts).

Fixes #184

[southpolesteve]

/bigbonk review

[pkg-pr-new]

Open in StackBlitz

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

commit: 6f43066

[yunus25jmi1]

@copilot The failing job encountered repeated 500 Internal Server Error responses from GitHub during git fetch operations, resulting in exit code 128. This is not an issue with your code or workflow configuration.

[yunus25jmi1]

@southpolesteve @elithrar kindly review the changes.

[yunus25jmi1]

kindly re-review the changes. There was a GitHub issue which failed the deploy examples/ Comment Preview URL

[southpolesteve]

/bigbonk review this again please!

[ask-bonk]

Review Summary

The fix addresses a real problem (issue #184): projects without "type": "module" in package.json break Vite's config loading on Node 22. The approach of reusing ensureESModule + renameCJSConfigs from utils/project.ts is sound, and the test coverage is thorough.

However, I have concerns about silently mutating the user's package.json and renaming their config files as a side effect of vinext dev / vinext build. This is a significant behavioral change that needs more careful handling. I also found some issues in the implementation.

Key Concerns

1. Silent file mutations during dev/build are dangerous. Renaming postcss.config.js → .cjs and rewriting package.json during a vinext dev invocation can surprise users. If the process crashes or is killed mid-write, it can leave the project in an inconsistent state (e.g., postcss.config.js renamed but type:module not yet added). Consider wrapping both operations atomically, or at minimum doing the package.json write before the renames so a partial failure is recoverable.

2. The warning message is misleading. It says "Run vinext init to make this change permanent" — but the change was already written to disk by the time the user sees this message. There's nothing impermanent about it. Either remove the misleading language or make the behavior truly non-persistent (e.g., set type:module as an env/flag that Vite reads, rather than mutating package.json).

3. Tests are thorough but don't test the actual ensureViteConfigCompatibility function. The new tests exercise ensureESModule and renameCJSConfigs in isolation (which already had tests in the same file). The new wrapper function ensureViteConfigCompatibility — which has its own logic (checking hasViteConfig(), reading and parsing package.json, the early-return conditions) — has no direct tests. This is the code most likely to have bugs.

4. Two describe blocks with near-identical tests. The PR adds both "ESM config compatibility — issue #184" and "ensureESModule + renameCJSConfigs — issue #184 compatibility" blocks that largely duplicate each other and also duplicate the existing ensureESModule and renameCJSConfigs describe blocks.

[ask-bonk]

Bug: ensureViteConfigCompatibility duplicates package.json parsing.

This function reads and parses package.json itself (lines 201-212), then calls ensureESModule(root) which reads and parses the same package.json again. More importantly, if ensureESModule returns false for any reason (file disappeared, parse error in its own try/catch), the if (added) check silently swallows the failure — the user gets no feedback that something went wrong after CJS configs were already renamed.

This redundant parsing also means there's a TOCTOU race: the function checks pkg.type === "module" at line 212, then ensureESModule checks it again. If something else modifies the file between the two reads, behavior is unpredictable.

Consider removing the duplicate parsing and just calling ensureESModule directly, letting it handle the idempotency check internally (which it already does):

Suggested change

	}
	function ensureViteConfigCompatibility(root: string): void {
	// Only act when there is a vite.config — auto-config mode sets
	// configFile: false and doesn't go through Vite's file-loading path.
	if (!hasViteConfig()) return;
	const pkgPath = path.join(root, "package.json");
	if (!fs.existsSync(pkgPath)) return;
	// Rename any `.js` CJS config files first so they don't break after we
	// add "type": "module".
	const renamed = renameCJSConfigs(root);
	for (const [oldName, newName] of renamed) {
	console.warn(
	` [vinext] Renamed ${oldName} → ${newName} (required for "type": "module")`
	);
	}
	// Add "type": "module" so Vite loads vite.config.ts as ESM.
	const added = ensureESModule(root);
	if (added) {
	console.warn(
	` [vinext] Added "type": "module" to package.json so Vite can load` +
	` vite.config.ts as ESM.\n` +
	` Run \`vinext init\` to review all project configuration.`
	);
	}
	}

[ask-bonk]

Misleading warning message. The change is already written to package.json by this point — it's not temporary or reversible. Telling users to "run vinext init to make this change permanent" implies the change isn't permanent yet, which is confusing.

Consider rewording to something like:

[vinext] Added "type": "module" to package.json (required for Vite ESM config loading).
  Run `vinext init` to review all project configuration.

[ask-bonk]

CJS config renames happen even when type:module is already set. The early return if (pkg.type === "module") return; at line 212 prevents this in the normal case, but if package.json parsing fails (line 207 catch), the function returns early before renames. That's fine. However, if someone has "type": "commonjs" explicitly set, this code will rename their CJS configs and add "type": "module" — overriding an explicit user choice. You might want to check for pkg.type !== undefined (any explicit value) rather than just pkg.type === "module".

[ask-bonk]

Duplicate test coverage. This describe("ESM config compatibility — issue #184") block adds tests for ensureESModule and renameCJSConfigs that are largely identical to the existing describe("ensureESModule") (line 860) and describe("renameCJSConfigs") (line 906) blocks, and also to the second new block describe("ensureESModule + renameCJSConfigs — issue #184 compatibility") (line 1868).

For example:

• "adds type:module when vite.config.ts is present" — same as existing test at line 861
• "is idempotent" — same as existing test at line 872
• "renames CJS postcss.config.js" — same as existing test at line 907

Consider removing one of the two new describe blocks entirely (they test the same functions) and focusing the remaining tests on scenarios not already covered — like the monorepo case and the "preserves all fields" case, which are genuinely new.

[ask-bonk]

hasViteConfig() uses process.cwd() but root is passed as a parameter. The hasViteConfig() function (defined at line 135) hardcodes process.cwd(), while this function accepts root as a parameter. If anyone ever calls ensureViteConfigCompatibility with a root different from process.cwd(), the vite config check will look in the wrong directory.

Currently this is safe because both call sites pass process.cwd(), but it's fragile. Consider using the hasViteConfig(root) from utils/project.ts instead, which accepts a root parameter:

Suggested change

	if (!hasViteConfig()) return;
	if (!hasViteConfig()) return;

Should be:

  // Import hasViteConfig from utils/project.ts which accepts root param
  // or inline the check here
  const hasConfig = 
    fs.existsSync(path.join(root, "vite.config.ts")) ||
    fs.existsSync(path.join(root, "vite.config.js")) ||
    fs.existsSync(path.join(root, "vite.config.mjs"));
  if (!hasConfig) return;

Note: There's already a hasViteConfig(root: string) exported from utils/project.ts (line 107) that does exactly this — you're already importing from that module.

[ask-bonk]

Review posted successfully. Here's a summary of the feedback I left on PR #197:

Review: REQUEST_CHANGES

The fix addresses a real problem (issue #184) and the approach of reusing shared utilities is sound. However, I identified several issues:

Implementation bugs/concerns:

1. Duplicate package.json parsing in ensureViteConfigCompatibility — it reads/parses the file, then ensureESModule reads/parses it again, creating a TOCTOU race
2. hasViteConfig() uses process.cwd() while the function accepts a root parameter — there's already a hasViteConfig(root) in utils/project.ts that should be used instead
3. Misleading warning message — says "run vinext init to make this change permanent" but the change was already permanently written to disk
4. No guard against explicit "type": "commonjs" — the code would override an explicit user choice
5. Non-atomic file mutations — if the process crashes between CJS renames and package.json write, the project is left in an inconsistent state

Test concerns:

• Two new describe blocks with near-identical tests that also duplicate existing test coverage
• The actual new function (ensureViteConfigCompatibility) is not directly tested — only the underlying utilities it calls

github run

[yunus25jmi1]

@southpolesteve Kindly review the latest changes. The workflow is all green.

[yunus25jmi1]

@hyoban Kindly review the latest changes. The workflow is all green.

[yunus25jmi1]

@james-elicx kindly review the PR.

[james-elicx]

/bigbonk review

[ask-bonk]

Review

The fix addresses a real problem and the approach of reusing ensureESModule + renameCJSConfigs from utils/project.ts is sound. I can see that the previous reviewer's feedback has been partially addressed — the hasViteConfig now correctly uses hasViteConfigInRoot(root) from utils/project.ts, the explicit "type": "commonjs" case is handled, and the warning message is improved.

However, there are still a couple of issues worth addressing:

1. Duplicate package.json parsing (minor, but real)

ensureViteConfigCompatibility reads and parses package.json (lines 201-209) to check early-return conditions, then calls ensureESModule(root) which reads and parses package.json again. This is harmless for correctness (the second read is idempotent), but it's wasted I/O. More importantly, it means the guard logic (the pkg.type !== undefined check at line 215) lives here while ensureESModule has its own guard (pkg.type === "module"). If someone later changes the guard in one place but not the other, behavior diverges.

Consider either:

• Removing the duplicate parsing and relying solely on ensureESModule's internal idempotency, OR
• Adding a type field check parameter to ensureESModule so the guard lives in one place

2. ensureViteConfigCompatibility is not directly tested

The new tests exercise ensureESModule and renameCJSConfigs in isolation (which already have thorough tests in the same file). The actual function added to cli.ts — ensureViteConfigCompatibility — is the one with the new logic (vite config check, package.json existence check, the type !== undefined guard for explicit CJS), and it has no direct test coverage. The test comment says "Simulate what ensureViteConfigCompatibility does" but then just calls the two underlying utilities directly.

I understand this function is private to cli.ts and harder to test in isolation, but it would be worth either: exporting it for testing, or adding an integration-level test that exercises the CLI path.

3. Test duplication with existing coverage

The new test block at line 1809 has significant overlap with the existing describe("ensureESModule") (line 860) and describe("renameCJSConfigs") (line 906) blocks. For example:

• "preserves all existing package.json fields" — near-identical to line 884
• "handles multiple CJS config files simultaneously" — near-identical to line 931

The two genuinely new tests are the monorepo workspace scenario and the full fix scenario (both utilities composed together). I'd suggest trimming the duplicates and keeping only the tests that add new coverage.

Overall the implementation is solid and the fix is correct. The issues above are cleanup-level — none are blockers.

[ask-bonk]

Minor: ensureESModule will re-read and re-parse package.json here, despite having already parsed it at line 206. This is harmless but redundant. If ensureESModule fails for any reason (returns false), this code silently proceeds after having already renamed CJS configs — the user could end up with renamed configs but no type:module, which is the inconsistent state the previous reviewer flagged.

One option: since you already have the parsed pkg object, write type:module directly here instead of delegating to ensureESModule:

Suggested change

	const added = ensureESModule(root);
	// Add "type": "module" so Vite loads vite.config.ts as ESM.
	try {
	pkg.type = "module";
	fs.writeFileSync(pkgPath, JSON.stringify(pkg, null, 2) + "\n", "utf-8");
	console.warn(
	` [vinext] Added "type": "module" to package.json (required for Vite ESM config loading).\n` +
	` Run \`vinext init\` to review all project configuration.`
	);
	} catch {
	// If we can't write, Vite will fail with a clearer error downstream.
	}

This eliminates the double read, and if the write fails, at least the CJS renames are still valid (they just mean .cjs extension with no semantic change since type:module wasn't added).

[ask-bonk]

This test ("preserves all existing package.json fields") is near-identical to the existing test at line 884. Consider removing it and keeping only the tests that provide new coverage — the monorepo scenario (line 1841) and the full integration scenario (line 1810) are the genuinely new ones.

[ask-bonk]

This test ("handles multiple CJS config files simultaneously") is near-identical to the existing test at line 931. Consider removing this duplicate.

[ask-bonk]

The comment says "Simulate what ensureViteConfigCompatibility does" — but it's calling the two utilities directly rather than testing the actual wrapper function. This means the wrapper's own logic (the vite config existence check, the pkg.type !== undefined guard for explicit CJS) has no test coverage.

If the function can't easily be exported from cli.ts, consider at minimum adding a test that verifies the "type": "commonjs" guard doesn't get overridden — that's the most important new behavior in the wrapper that isn't covered by existing tests of the underlying utilities.

[ask-bonk]

Review posted successfully. Here's a summary of the feedback I left on PR #197:

Overall: The fix is correct and addresses a real problem (issue #184). The approach of reusing shared utilities from utils/project.ts is sound, and the previous reviewer's key concerns have been addressed (proper hasViteConfig(root) usage, explicit "type": "commonjs" guard, improved warning message).

Three remaining items (cleanup-level, not blockers):

1. Duplicate package.json parsing — ensureViteConfigCompatibility parses package.json at line 206, then ensureESModule parses it again. This creates divergent guard logic in two places. Suggested inlining the write directly using the already-parsed pkg object instead of delegating to ensureESModule.

2. ensureViteConfigCompatibility itself is untested — The tests call ensureESModule and renameCJSConfigs directly rather than testing the wrapper function, which means the new guard logic (vite config check, type !== undefined for explicit CJS) has no coverage.

3. Test duplication — Two of the four new tests are near-duplicates of existing tests. Suggested keeping only the monorepo workspace scenario and the full integration scenario, which provide genuinely new coverage.

github run