-
Notifications
You must be signed in to change notification settings - Fork 210
fix(cli): ensure type:module before Vite loads vite.config.ts (fixes #184) #197
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,7 @@ import { deploy as runDeploy, parseDeployArgs } from "./deploy.js"; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { runCheck, formatReport } from "./check.js"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { init as runInit } from "./init.js"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { loadDotenv } from "./config/dotenv.js"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { ensureESModule, renameCJSConfigs, hasViteConfig as hasViteConfigInRoot } from "./utils/project.js"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // ─── Resolve Vite from the project root ──────────────────────────────────────── | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -177,6 +178,61 @@ function buildViteConfig(overrides: Record<string, unknown> = {}) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return config; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Ensure the project's package.json has `"type": "module"` before Vite loads | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * the vite.config.ts. This prevents the esbuild CJS-bundling path that Vite | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * takes for projects without `"type": "module"`, which produces a `.mjs` temp | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * file containing `require()` calls — calls that fail on Node 22 when | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * targeting pure-ESM packages like `@cloudflare/vite-plugin`. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * This mirrors what `vinext init` does, but is applied lazily at dev/build | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * time for projects that were set up before `vinext init` added the step, or | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * that were migrated manually. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Side effects: may rename `.js` config files like `postcss.config.js` to | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * `.cjs` to avoid CJS/ESM confusion after `"type": "module"` is added (the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * same rename logic used by `vinext init`). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 (!hasViteConfigInRoot(root)) return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const pkgPath = path.join(root, "package.json"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!fs.existsSync(pkgPath)) return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let pkg: Record<string, unknown>; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pkg = JSON.parse(fs.readFileSync(pkgPath, "utf-8")); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Already correct — nothing to do. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (pkg.type === "module") return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Respect explicit "type": "commonjs" — the user chose this deliberately. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (pkg.type !== undefined) 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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. Minor: One option: since you already have the parsed
Suggested change
This eliminates the double read, and if the write fails, at least the CJS renames are still valid (they just mean |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (added) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.warn( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ` [vinext] Added "type": "module" to package.json (required for Vite ESM config loading).\n` + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ` Run \`vinext init\` to review all project configuration.` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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 function reads and parses This redundant parsing also means there's a TOCTOU race: the function checks Consider removing the duplicate parsing and just calling
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // ─── Commands ───────────────────────────────────────────────────────────────── | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async function dev() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -188,6 +244,11 @@ async function dev() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mode: "development", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Ensure "type": "module" in package.json before Vite loads vite.config.ts. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Without this, Vite bundles the config as CJS and tries require() on pure-ESM | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // packages like @cloudflare/vite-plugin, which fails on Node 22. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ensureViteConfigCompatibility(process.cwd()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const vite = await loadVite(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const port = parsed.port ?? 3000; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -213,6 +274,11 @@ async function buildApp() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mode: "production", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Ensure "type": "module" in package.json before Vite loads vite.config.ts. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Without this, Vite bundles the config as CJS and tries require() on pure-ESM | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // packages like @cloudflare/vite-plugin, which fails on Node 22. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ensureViteConfigCompatibility(process.cwd()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const vite = await loadVite(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log(`\n vinext build (Vite ${getViteVersion()})\n`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1790,3 +1790,118 @@ describe("Cloudflare closeBundle lazy chunk injection", () => { | |
| expect(lazy).not.toContain("assets/framework.js"); | ||
| }); | ||
| }); | ||
|
|
||
| // ─── ESM config compatibility (issue #184) ────────────────────────────────── | ||
| // | ||
| // Regression tests for ensureViteConfigCompatibility() — the wrapper in cli.ts | ||
| // that calls ensureESModule + renameCJSConfigs before Vite loads the config. | ||
| // | ||
|
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. Duplicate test coverage. This For example:
Consider removing one of the two new |
||
| // Scenario from issue #184: | ||
| // Project has vite.config.ts importing @cloudflare/vite-plugin. | ||
| // package.json lacks "type":"module". | ||
| // Vite bundles the config with esbuild → outputs CJS require() for the plugin. | ||
| // Node 22 throws: Error: Dynamic require of "…index.mjs" is not supported | ||
| // | ||
| // The underlying ensureESModule() and renameCJSConfigs() are already tested | ||
| // above. These tests cover the unique scenarios from the issue and the | ||
| // integration of both functions together. | ||
|
|
||
| describe("ESM config compatibility — issue #184", () => { | ||
| it("full fix scenario: renames CJS postcss config + adds type:module", () => { | ||
| // Yarn 1 monorepo scenario: no "type":"module", CJS postcss config | ||
| writeFile( | ||
| tmpDir, | ||
| "package.json", | ||
| JSON.stringify({ name: "web", version: "1.0.0" }), | ||
| ); | ||
| writeFile( | ||
| tmpDir, | ||
| "vite.config.ts", | ||
| 'import { cloudflare } from "@cloudflare/vite-plugin";\nexport default { plugins: [cloudflare()] };', | ||
| ); | ||
| writeFile( | ||
| tmpDir, | ||
| "postcss.config.js", | ||
| "module.exports = { plugins: { tailwindcss: {}, autoprefixer: {} } };", | ||
| ); | ||
|
|
||
| // Simulate what ensureViteConfigCompatibility does | ||
|
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. The comment says "Simulate what If the function can't easily be exported from |
||
| const renamed = renameCJSConfigs(tmpDir); | ||
| const added = ensureESModule(tmpDir); | ||
|
|
||
| expect(added).toBe(true); | ||
| const pkg = JSON.parse(fs.readFileSync(path.join(tmpDir, "package.json"), "utf-8")); | ||
| expect(pkg.type).toBe("module"); | ||
|
|
||
| expect(renamed).toEqual([["postcss.config.js", "postcss.config.cjs"]]); | ||
| expect(fs.existsSync(path.join(tmpDir, "postcss.config.cjs"))).toBe(true); | ||
| expect(fs.existsSync(path.join(tmpDir, "postcss.config.js"))).toBe(false); | ||
| }); | ||
|
|
||
| it("handles a workspaces monorepo: only updates the leaf package.json", () => { | ||
| const webDir = path.join(tmpDir, "apps", "web"); | ||
| fs.mkdirSync(webDir, { recursive: true }); | ||
|
|
||
| // Root package.json (CJS) | ||
| writeFile(tmpDir, "package.json", JSON.stringify({ name: "monorepo", workspaces: ["apps/*"] })); | ||
| // Workspace package.json (no type:module) | ||
| writeFile(webDir, "package.json", JSON.stringify({ name: "web", version: "1.0.0" })); | ||
| writeFile(webDir, "vite.config.ts", "export default {};"); | ||
|
|
||
| ensureESModule(webDir); | ||
|
|
||
| const rootPkg = JSON.parse(fs.readFileSync(path.join(tmpDir, "package.json"), "utf-8")); | ||
| const webPkg = JSON.parse(fs.readFileSync(path.join(webDir, "package.json"), "utf-8")); | ||
|
|
||
| // Only the workspace package should be modified | ||
| expect(webPkg.type).toBe("module"); | ||
| expect(rootPkg.type).toBeUndefined(); | ||
| }); | ||
|
|
||
| it("preserves all existing package.json fields when adding type:module", () => { | ||
|
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. 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. |
||
| writeFile( | ||
| tmpDir, | ||
| "package.json", | ||
| JSON.stringify({ | ||
| name: "web", | ||
| version: "0.1.0", | ||
| private: true, | ||
| scripts: { dev: "vinext dev", build: "vinext build" }, | ||
| dependencies: { react: "^19.0.0", "react-dom": "^19.0.0" }, | ||
| devDependencies: { vinext: "^0.0.13", "@cloudflare/vite-plugin": "^1.0.0" }, | ||
| }), | ||
| ); | ||
|
|
||
| ensureESModule(tmpDir); | ||
|
|
||
| const pkg = JSON.parse(fs.readFileSync(path.join(tmpDir, "package.json"), "utf-8")); | ||
| expect(pkg.type).toBe("module"); | ||
| expect(pkg.name).toBe("web"); | ||
| expect(pkg.version).toBe("0.1.0"); | ||
| expect(pkg.private).toBe(true); | ||
| expect(pkg.scripts.build).toBe("vinext build"); | ||
| expect(pkg.dependencies.react).toBe("^19.0.0"); | ||
| expect(pkg.devDependencies.vinext).toBe("^0.0.13"); | ||
| }); | ||
|
|
||
| it("handles multiple CJS config files simultaneously", () => { | ||
|
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. This test ("handles multiple CJS config files simultaneously") is near-identical to the existing test at line 931. Consider removing this duplicate. |
||
| writeFile( | ||
| tmpDir, | ||
| "package.json", | ||
| JSON.stringify({ name: "my-app", version: "1.0.0" }), | ||
| ); | ||
| writeFile(tmpDir, "postcss.config.js", "module.exports = {};"); | ||
| writeFile(tmpDir, "tailwind.config.js", "module.exports = {};"); | ||
| writeFile(tmpDir, ".eslintrc.js", "module.exports = {};"); | ||
|
|
||
| const renamed = renameCJSConfigs(tmpDir); | ||
| ensureESModule(tmpDir); | ||
|
|
||
| expect(renamed).toHaveLength(3); | ||
| expect(fs.existsSync(path.join(tmpDir, "postcss.config.cjs"))).toBe(true); | ||
| expect(fs.existsSync(path.join(tmpDir, "tailwind.config.cjs"))).toBe(true); | ||
| expect(fs.existsSync(path.join(tmpDir, ".eslintrc.cjs"))).toBe(true); | ||
| const pkg = JSON.parse(fs.readFileSync(path.join(tmpDir, "package.json"), "utf-8")); | ||
| expect(pkg.type).toBe("module"); | ||
| }); | ||
| }); | ||
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.
CJS config renames happen even when
type:moduleis already set. The early returnif (pkg.type === "module") return;at line 212 prevents this in the normal case, but ifpackage.jsonparsing 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 forpkg.type !== undefined(any explicit value) rather than justpkg.type === "module".