From f30cd75a19ff8c971f77df4562eed09d0edee244 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Wed, 15 Nov 2023 10:46:47 +0000 Subject: [PATCH 01/24] fix: ensure C3 shell scripts work on Windows Our use of `shell-quote` was causing problems on Windows where it was escaping character (such as `@`) by placing a backslash in front. This made Windows think that such path arguments, were at the root. For example, `npm install -D @cloudflare/workers-types` was being converted to `npm install -D \@cloudflare/workers-types`, which resulted in errors like: ``` npm ERR! enoent ENOENT: no such file or directory, open 'D:\@cloudflare\workers-types\package.json' ``` Now we just rely directly on the Node.js `spawn` API to avoid any shell quoting concerns. This has resulted in a slightly less streamlined experience for people writing C3 plugins, but has the benefit that the developer doesn't have to worry about quoting spawn arguments. Closes https://github.com/cloudflare/workers-sdk/issues/4282 --- .changeset/angry-walls-cheer.md | 23 +++++++++ .../create-cloudflare/e2e-tests/cli.test.ts | 13 +++-- packages/create-cloudflare/package.json | 2 - packages/create-cloudflare/src/cli.ts | 12 ++--- packages/create-cloudflare/src/common.ts | 39 ++++++++------- .../src/frameworks/angular/index.ts | 4 +- .../src/frameworks/astro/index.ts | 4 +- .../src/frameworks/docusaurus/index.ts | 2 +- .../src/frameworks/gatsby/index.ts | 2 +- .../src/frameworks/hono/index.ts | 9 ++-- .../src/frameworks/next/index.ts | 2 +- .../src/frameworks/nuxt/index.ts | 11 +++-- .../src/frameworks/qwik/index.ts | 6 +-- .../src/frameworks/react/index.ts | 2 +- .../src/frameworks/remix/index.ts | 9 ++-- .../src/frameworks/solid/index.ts | 2 +- .../src/frameworks/svelte/index.ts | 2 +- .../src/frameworks/vue/index.ts | 2 +- .../src/helpers/__tests__/command.test.ts | 40 +++++----------- .../create-cloudflare/src/helpers/command.ts | 48 ++++++------------- .../create-cloudflare/src/helpers/packages.ts | 12 ++--- .../src/helpers/shell-quote.ts | 47 ------------------ packages/create-cloudflare/src/pages.ts | 37 +++++++++----- packages/create-cloudflare/src/workers.ts | 10 +++- pnpm-lock.yaml | 6 --- 25 files changed, 158 insertions(+), 188 deletions(-) create mode 100644 .changeset/angry-walls-cheer.md delete mode 100644 packages/create-cloudflare/src/helpers/shell-quote.ts diff --git a/.changeset/angry-walls-cheer.md b/.changeset/angry-walls-cheer.md new file mode 100644 index 00000000000..5e067722f46 --- /dev/null +++ b/.changeset/angry-walls-cheer.md @@ -0,0 +1,23 @@ +--- +"create-cloudflare": patch +--- + +fix: ensure shell scripts work on Windows + +Our use of `shell-quote` was causing problems on Windows where it was +escaping character (such as `@`) by placing a backslash in front. +This made Windows think that such path arguments, were at the root. + +For example, `npm install -D @cloudflare/workers-types` was being converted to +`npm install -D \@cloudflare/workers-types`, which resulted in errors like: + +``` +npm ERR! enoent ENOENT: no such file or directory, open 'D:\@cloudflare\workers-types\package.json' +``` + +Now we just rely directly on the Node.js `spawn` API to avoid any shell quoting +concerns. This has resulted in a slightly less streamlined experience for people +writing C3 plugins, but has the benefit that the developer doesn't have to worry +about quoting spawn arguments. + +Closes https://github.com/cloudflare/workers-sdk/issues/4282 diff --git a/packages/create-cloudflare/e2e-tests/cli.test.ts b/packages/create-cloudflare/e2e-tests/cli.test.ts index f134094f8cf..fa5389f7497 100644 --- a/packages/create-cloudflare/e2e-tests/cli.test.ts +++ b/packages/create-cloudflare/e2e-tests/cli.test.ts @@ -10,7 +10,6 @@ import { beforeAll, } from "vitest"; import { version } from "../package.json"; -import * as shellquote from "../src/helpers/shell-quote"; import { frameworkToTest } from "./frameworkToTest"; import { isQuarantineMode, keys, recreateLogFolder, runC3 } from "./helpers"; import type { Suite } from "vitest"; @@ -43,15 +42,19 @@ describe.skipIf(frameworkToTest || isQuarantineMode())( }); test("--version with positionals", async (ctx) => { - const argv = shellquote.parse("foo bar baz --version"); + const argv = ["foo", "bar", "baz", "--version"]; const { output } = await runC3({ ctx, argv }); expect(output).toEqual(version); }); test("--version with flags", async (ctx) => { - const argv = shellquote.parse( - "foo --type webFramework --no-deploy --version" - ); + const argv = [ + "foo", + "--type", + "webFramework", + "--no-deploy", + "--version", + ]; const { output } = await runC3({ ctx, argv }); expect(output).toEqual(version); }); diff --git a/packages/create-cloudflare/package.json b/packages/create-cloudflare/package.json index 325d2e86f11..5270fccd712 100644 --- a/packages/create-cloudflare/package.json +++ b/packages/create-cloudflare/package.json @@ -55,7 +55,6 @@ "@types/esprima": "^4.0.3", "@types/node": "^18.15.3", "@types/semver": "^7.5.1", - "@types/shell-quote": "^1.7.2", "@types/which-pm-runs": "^1.0.0", "@types/yargs": "^17.0.22", "@typescript-eslint/eslint-plugin": "^5.55.0", @@ -72,7 +71,6 @@ "pnpm": "^8.10.0", "recast": "^0.22.0", "semver": "^7.5.1", - "shell-quote": "^1.8.1", "typescript": "^5.0.2", "undici": "5.20.0", "vite-tsconfig-paths": "^4.0.8", diff --git a/packages/create-cloudflare/src/cli.ts b/packages/create-cloudflare/src/cli.ts index 6bf658f3885..a1d506c0e9f 100644 --- a/packages/create-cloudflare/src/cli.ts +++ b/packages/create-cloudflare/src/cli.ts @@ -13,7 +13,6 @@ import { detectPackageManager } from "helpers/packages"; import semver from "semver"; import { version } from "../package.json"; import { validateProjectDirectory } from "./common"; -import * as shellquote from "./helpers/shell-quote"; import { templateMap } from "./templateMap"; import type { C3Args } from "types"; @@ -43,7 +42,7 @@ const isUpdateAvailable = async () => { const s = spinner(spinnerFrames.vertical, blue); s.start("Checking if a newer version is available"); const latestVersion = await runCommand( - `npm info create-cloudflare@latest dist-tags.latest`, + ["npm", "info", "create-cloudflare@latest", "dist-tags.latest"], { silent: true, useSpinner: false } ); s.stop(); @@ -60,12 +59,11 @@ export const runLatest = async () => { // the parsing logic of `npm create` requires `--` to be supplied // before any flags intended for the target command. - const argString = - npm === "npm" - ? `-- ${shellquote.quote(args)}` - : `${shellquote.quote(args)}`; + if (npm === "npm") { + args.unshift("--"); + } - await runCommand(`${npm} create cloudflare@latest ${argString}`); + await runCommand([npm, "create", "cloudflare@latest", ...args]); }; // Entrypoint to c3 diff --git a/packages/create-cloudflare/src/common.ts b/packages/create-cloudflare/src/common.ts index 7c99b69baa2..590169c2904 100644 --- a/packages/create-cloudflare/src/common.ts +++ b/packages/create-cloudflare/src/common.ts @@ -27,7 +27,6 @@ import { detectPackageManager } from "helpers/packages"; import { poll } from "helpers/poll"; import { version as wranglerVersion } from "wrangler/package.json"; import { version } from "../package.json"; -import * as shellquote from "./helpers/shell-quote"; import type { C3Args, PagesGeneratorContext } from "types"; const { name, npm } = detectPackageManager(); @@ -178,10 +177,8 @@ export const runDeploy = async (ctx: PagesGeneratorContext) => { ...(ctx.framework?.commitMessage && !insideGitRepo ? [ ...(name === "npm" ? ["--"] : []), - `--commit-message="${ctx.framework.commitMessage.replaceAll( - '"', - '\\"' - )}"`, + "--commit-message", + prepareCommitMessage(ctx.framework.commitMessage), ] : []), ]; @@ -192,7 +189,7 @@ export const runDeploy = async (ctx: PagesGeneratorContext) => { env: { CLOUDFLARE_ACCOUNT_ID: ctx.account.id, NODE_ENV: "production" }, startText: "Deploying your application", doneText: `${brandColor("deployed")} ${dim( - `via \`${shellquote.quote(baseDeployCmd)}\`` + `via \`${baseDeployCmd.join(" ")}\`` )}`, }); @@ -254,7 +251,7 @@ export const printSummary = async (ctx: PagesGeneratorContext) => { const nextSteps = [ [ `Navigate to the new directory`, - `cd ${shellquote.quote([relative(ctx.originalCWD, ctx.project.path)])}`, + `cd ${relative(ctx.originalCWD, ctx.project.path)}`, ], [ `Run the development server`, @@ -382,7 +379,10 @@ export const gitCommit = async (ctx: PagesGeneratorContext) => { await runCommands({ silent: true, cwd: ctx.project.path, - commands: ["git add .", ["git", "commit", "-m", commitMessage]], + commands: [ + ["git", "add", "."], + ["git", "commit", "-m", commitMessage], + ], startText: "Committing new files", doneText: `${brandColor("git")} ${dim(`commit`)}`, }); @@ -435,7 +435,7 @@ const createCommitMessage = async (ctx: PagesGeneratorContext) => { */ async function getGitVersion() { try { - const rawGitVersion = await runCommand("git --version", { + const rawGitVersion = await runCommand(["git", "--version"], { useSpinner: false, silent: true, }); @@ -456,13 +456,13 @@ export async function isGitInstalled() { export async function isGitConfigured() { try { - const userName = await runCommand("git config user.name", { + const userName = await runCommand(["git", "config", "user.name"], { useSpinner: false, silent: true, }); if (!userName) return false; - const email = await runCommand("git config user.email", { + const email = await runCommand(["git", "config", "user.email"], { useSpinner: false, silent: true, }); @@ -480,7 +480,7 @@ export async function isGitConfigured() { */ export async function isInsideGitRepo(cwd: string) { try { - const output = await runCommand("git status", { + const output = await runCommand(["git", "status"], { cwd, useSpinner: false, silent: true, @@ -503,18 +503,18 @@ export async function initializeGit(cwd: string) { try { // Get the default init branch name const defaultBranchName = await runCommand( - "git config --get init.defaultBranch", + ["git", "config", "--get", "init.defaultBranch"], { useSpinner: false, silent: true, cwd } ); // Try to create the repository with the HEAD branch of defaultBranchName ?? `main`. await runCommand( - `git init --initial-branch ${defaultBranchName.trim() ?? "main"}`, // branch names can't contain spaces, so this is safe + ["git", "init", "--initial-branch", defaultBranchName.trim() ?? "main"], // branch names can't contain spaces, so this is safe { useSpinner: false, silent: true, cwd } ); } catch { // Unable to create the repo with a HEAD branch name, so just fall back to the default. - await runCommand(`git init`, { useSpinner: false, silent: true, cwd }); + await runCommand(["git", "init"], { useSpinner: false, silent: true, cwd }); } } @@ -522,7 +522,7 @@ export async function getProductionBranch(cwd: string) { try { const productionBranch = await runCommand( // "git branch --show-current", // git@^2.22 - "git rev-parse --abbrev-ref HEAD", // git@^1.6.3 + ["git", "rev-parse", "--abbrev-ref", "HEAD"], // git@^1.6.3 { silent: true, cwd, @@ -536,3 +536,10 @@ export async function getProductionBranch(cwd: string) { return "main"; } + +/** + * Ensure that the commit message has newlines etc properly escaped. + */ +function prepareCommitMessage(commitMessage: string): string { + return JSON.stringify(commitMessage); +} diff --git a/packages/create-cloudflare/src/frameworks/angular/index.ts b/packages/create-cloudflare/src/frameworks/angular/index.ts index 27657abbf91..49224fd2073 100644 --- a/packages/create-cloudflare/src/frameworks/angular/index.ts +++ b/packages/create-cloudflare/src/frameworks/angular/index.ts @@ -11,7 +11,7 @@ import type { FrameworkConfig, PagesGeneratorContext } from "types"; const { npm } = detectPackageManager(); const generate = async (ctx: PagesGeneratorContext) => { - await runFrameworkGenerator(ctx, `${ctx.project.name} --ssr`); + await runFrameworkGenerator(ctx, [ctx.project.name, "--ssr"]); logRaw(""); }; @@ -35,7 +35,7 @@ const config: FrameworkConfig = { }), deployCommand: "deploy", devCommand: "start", - testFlags: ["--ssr", "--style", "sass"], + testFlags: ["--style", "sass"], }; export default config; diff --git a/packages/create-cloudflare/src/frameworks/astro/index.ts b/packages/create-cloudflare/src/frameworks/astro/index.ts index a4b5b11e319..7c3cedaea29 100644 --- a/packages/create-cloudflare/src/frameworks/astro/index.ts +++ b/packages/create-cloudflare/src/frameworks/astro/index.ts @@ -8,7 +8,7 @@ import type { FrameworkConfig, PagesGeneratorContext } from "types"; const { npx } = detectPackageManager(); const generate = async (ctx: PagesGeneratorContext) => { - await runFrameworkGenerator(ctx, `${ctx.project.name} --no-install`); + await runFrameworkGenerator(ctx, [ctx.project.name, "--no-install"]); logRaw(""); // newline }; @@ -20,7 +20,7 @@ const configure = async (ctx: PagesGeneratorContext) => { // Need to ensure install first so `astro` works await npmInstall(); - await runCommand(`${npx} astro add cloudflare -y`, { + await runCommand([npx, "astro", "add", "cloudflare", "-y"], { silent: true, startText: "Installing adapter", doneText: `${brandColor("installed")} ${dim( diff --git a/packages/create-cloudflare/src/frameworks/docusaurus/index.ts b/packages/create-cloudflare/src/frameworks/docusaurus/index.ts index af1241e14b4..29fae9cbf91 100644 --- a/packages/create-cloudflare/src/frameworks/docusaurus/index.ts +++ b/packages/create-cloudflare/src/frameworks/docusaurus/index.ts @@ -6,7 +6,7 @@ import type { PagesGeneratorContext, FrameworkConfig } from "types"; const { npm } = detectPackageManager(); const generate = async (ctx: PagesGeneratorContext) => { - await runFrameworkGenerator(ctx, `${ctx.project.name} classic`); + await runFrameworkGenerator(ctx, [ctx.project.name, "classic"]); }; const config: FrameworkConfig = { diff --git a/packages/create-cloudflare/src/frameworks/gatsby/index.ts b/packages/create-cloudflare/src/frameworks/gatsby/index.ts index 4ba79eb152f..df8834196b6 100644 --- a/packages/create-cloudflare/src/frameworks/gatsby/index.ts +++ b/packages/create-cloudflare/src/frameworks/gatsby/index.ts @@ -26,7 +26,7 @@ const generate = async (ctx: PagesGeneratorContext) => { }); } - await runFrameworkGenerator(ctx, `new ${ctx.project.name} ${templateUrl}`); + await runFrameworkGenerator(ctx, ["new", ctx.project.name, templateUrl]); }; const config: FrameworkConfig = { diff --git a/packages/create-cloudflare/src/frameworks/hono/index.ts b/packages/create-cloudflare/src/frameworks/hono/index.ts index c4bcf18b9fc..05ce6c7f38c 100644 --- a/packages/create-cloudflare/src/frameworks/hono/index.ts +++ b/packages/create-cloudflare/src/frameworks/hono/index.ts @@ -3,10 +3,11 @@ import { runFrameworkGenerator } from "helpers/command"; import type { FrameworkConfig, PagesGeneratorContext } from "types"; const generate = async (ctx: PagesGeneratorContext) => { - await runFrameworkGenerator( - ctx, - `${ctx.project.name} --template cloudflare-workers` - ); + await runFrameworkGenerator(ctx, [ + ctx.project.name, + "--template", + "cloudflare-workers", + ]); logRaw(""); // newline }; diff --git a/packages/create-cloudflare/src/frameworks/next/index.ts b/packages/create-cloudflare/src/frameworks/next/index.ts index 4a30784c41b..8a44ec750e5 100644 --- a/packages/create-cloudflare/src/frameworks/next/index.ts +++ b/packages/create-cloudflare/src/frameworks/next/index.ts @@ -26,7 +26,7 @@ const { npm, npx } = detectPackageManager(); const generate = async (ctx: PagesGeneratorContext) => { const projectName = ctx.project.name; - await runFrameworkGenerator(ctx, `${projectName}`); + await runFrameworkGenerator(ctx, [projectName]); }; const getApiTemplate = ( diff --git a/packages/create-cloudflare/src/frameworks/nuxt/index.ts b/packages/create-cloudflare/src/frameworks/nuxt/index.ts index 96a0898cf30..377c0636238 100644 --- a/packages/create-cloudflare/src/frameworks/nuxt/index.ts +++ b/packages/create-cloudflare/src/frameworks/nuxt/index.ts @@ -9,10 +9,13 @@ const { npm } = detectPackageManager(); const generate = async (ctx: PagesGeneratorContext) => { const gitFlag = ctx.args.git ? `--gitInit` : `--no-gitInit`; - await runFrameworkGenerator( - ctx, - `init ${ctx.project.name} --packageManager ${npm} ${gitFlag}` - ); + await runFrameworkGenerator(ctx, [ + "init", + ctx.project.name, + "--packageManager", + npm, + gitFlag, + ]); logRaw(""); // newline }; diff --git a/packages/create-cloudflare/src/frameworks/qwik/index.ts b/packages/create-cloudflare/src/frameworks/qwik/index.ts index 19441c6dcac..b9a2cc38ea0 100644 --- a/packages/create-cloudflare/src/frameworks/qwik/index.ts +++ b/packages/create-cloudflare/src/frameworks/qwik/index.ts @@ -7,7 +7,7 @@ import type { FrameworkConfig, PagesGeneratorContext } from "types"; const { npm, npx } = detectPackageManager(); const generate = async (ctx: PagesGeneratorContext) => { - await runFrameworkGenerator(ctx, `basic ${ctx.project.name}`); + await runFrameworkGenerator(ctx, ["basic", ctx.project.name]); }; const configure = async (ctx: PagesGeneratorContext) => { @@ -16,8 +16,8 @@ const configure = async (ctx: PagesGeneratorContext) => { await npmInstall(); // Add the pages integration - const cmd = `${npx} qwik add cloudflare-pages`; - endSection(`Running ${cmd}`); + const cmd = [npx, "qwik", "add", "cloudflare-pages"]; + endSection(`Running ${cmd.join(" ")}`); await runCommand(cmd); }; diff --git a/packages/create-cloudflare/src/frameworks/react/index.ts b/packages/create-cloudflare/src/frameworks/react/index.ts index f6e83181a3c..116993d21db 100644 --- a/packages/create-cloudflare/src/frameworks/react/index.ts +++ b/packages/create-cloudflare/src/frameworks/react/index.ts @@ -7,7 +7,7 @@ import type { FrameworkConfig, PagesGeneratorContext } from "types"; const { npm } = detectPackageManager(); const generate = async (ctx: PagesGeneratorContext) => { - await runFrameworkGenerator(ctx, `${ctx.project.name}`); + await runFrameworkGenerator(ctx, [ctx.project.name]); logRaw(""); }; diff --git a/packages/create-cloudflare/src/frameworks/remix/index.ts b/packages/create-cloudflare/src/frameworks/remix/index.ts index af9d006e090..6ddaa2f38e7 100644 --- a/packages/create-cloudflare/src/frameworks/remix/index.ts +++ b/packages/create-cloudflare/src/frameworks/remix/index.ts @@ -6,10 +6,11 @@ import type { FrameworkConfig, PagesGeneratorContext } from "types"; const { npm } = detectPackageManager(); const generate = async (ctx: PagesGeneratorContext) => { - await runFrameworkGenerator( - ctx, - `${ctx.project.name} --template https://github.com/remix-run/remix/tree/main/templates/cloudflare-pages` - ); + await runFrameworkGenerator(ctx, [ + ctx.project.name, + "--template", + "https://github.com/remix-run/remix/tree/main/templates/cloudflare-pages", + ]); logRaw(""); // newline }; diff --git a/packages/create-cloudflare/src/frameworks/solid/index.ts b/packages/create-cloudflare/src/frameworks/solid/index.ts index 736467817e3..3ca24e99cf7 100644 --- a/packages/create-cloudflare/src/frameworks/solid/index.ts +++ b/packages/create-cloudflare/src/frameworks/solid/index.ts @@ -10,7 +10,7 @@ const { npm } = detectPackageManager(); const generate = async (ctx: PagesGeneratorContext) => { // Run the create-solid command - await runFrameworkGenerator(ctx, `${ctx.project.name}`); + await runFrameworkGenerator(ctx, [ctx.project.name]); logRaw(""); }; diff --git a/packages/create-cloudflare/src/frameworks/svelte/index.ts b/packages/create-cloudflare/src/frameworks/svelte/index.ts index 1acc8c3c4d5..e48061ad950 100644 --- a/packages/create-cloudflare/src/frameworks/svelte/index.ts +++ b/packages/create-cloudflare/src/frameworks/svelte/index.ts @@ -15,7 +15,7 @@ import type { FrameworkConfig, PagesGeneratorContext } from "types"; const { npm } = detectPackageManager(); const generate = async (ctx: PagesGeneratorContext) => { - await runFrameworkGenerator(ctx, `${ctx.project.name}`); + await runFrameworkGenerator(ctx, [ctx.project.name]); logRaw(""); }; diff --git a/packages/create-cloudflare/src/frameworks/vue/index.ts b/packages/create-cloudflare/src/frameworks/vue/index.ts index 0049ab8d8f0..0cd4a73179f 100644 --- a/packages/create-cloudflare/src/frameworks/vue/index.ts +++ b/packages/create-cloudflare/src/frameworks/vue/index.ts @@ -6,7 +6,7 @@ import type { PagesGeneratorContext, FrameworkConfig } from "types"; const { npm } = detectPackageManager(); const generate = async (ctx: PagesGeneratorContext) => { - await runFrameworkGenerator(ctx, `${ctx.project.name}`); + await runFrameworkGenerator(ctx, [ctx.project.name]); }; const config: FrameworkConfig = { diff --git a/packages/create-cloudflare/src/helpers/__tests__/command.test.ts b/packages/create-cloudflare/src/helpers/__tests__/command.test.ts index 5aaeb276109..d8a512c411f 100644 --- a/packages/create-cloudflare/src/helpers/__tests__/command.test.ts +++ b/packages/create-cloudflare/src/helpers/__tests__/command.test.ts @@ -9,7 +9,6 @@ import { npmInstall, runCommand, } from "../command"; -import * as shellquote from "../shell-quote"; // We can change how the mock spawn works by setting these variables let spawnResultCode = 0; @@ -57,17 +56,8 @@ describe("Command Helpers", () => { })); }); - const expectSpawnWith = (cmd: string) => { - const [command, ...args] = shellquote.parse(cmd); - - expect(spawn).toHaveBeenCalledWith(command, args, { - stdio: "inherit", - env: process.env, - }); - }; - const expectSilentSpawnWith = (cmd: string) => { - const [command, ...args] = shellquote.parse(cmd); + const [command, ...args] = cmd.split(" "); expect(spawn).toHaveBeenCalledWith(command, args, { stdio: "pipe", @@ -76,17 +66,11 @@ describe("Command Helpers", () => { }; test("runCommand", async () => { - await runCommand("ls -l"); - expectSpawnWith("ls -l"); - - await runCommand(" ls -l "); - expectSpawnWith("ls -l"); - - await runCommand(" ls -l "); - expectSpawnWith("ls -l"); - - await runCommand(" ls \t -l "); - expectSpawnWith("ls -l"); + await runCommand(["ls", "-l"]); + expect(spawn).toHaveBeenCalledWith("ls", ["-l"], { + stdio: "inherit", + env: process.env, + }); }); test("installWrangler", async () => { @@ -121,7 +105,7 @@ describe("Command Helpers", () => { test("npm", () => { expect(pm.npm).toBe("npm"); expect(pm.npx).toBe("npx"); - expect(pm.dlx).toBe("npx"); + expect(pm.dlx).toEqual(["npx"]); }); test("pnpm", () => { @@ -132,7 +116,7 @@ describe("Command Helpers", () => { pm = detectPackageManager(); expect(pm.npm).toBe("pnpm"); expect(pm.npx).toBe("pnpm"); - expect(pm.dlx).toBe("pnpm dlx"); + expect(pm.dlx).toEqual(["pnpm", "dlx"]); vi.mocked(whichPMRuns).mockReturnValue({ name: "pnpm", @@ -141,7 +125,7 @@ describe("Command Helpers", () => { pm = detectPackageManager(); expect(pm.npm).toBe("pnpm"); expect(pm.npx).toBe("pnpm"); - expect(pm.dlx).toBe("pnpm dlx"); + expect(pm.dlx).toEqual(["pnpm", "dlx"]); vi.mocked(whichPMRuns).mockReturnValue({ name: "pnpm", @@ -150,7 +134,7 @@ describe("Command Helpers", () => { pm = detectPackageManager(); expect(pm.npm).toBe("pnpm"); expect(pm.npx).toBe("pnpx"); - expect(pm.dlx).toBe("pnpx"); + expect(pm.dlx).toEqual(["pnpx"]); }); test("yarn", () => { @@ -161,7 +145,7 @@ describe("Command Helpers", () => { pm = detectPackageManager(); expect(pm.npm).toBe("yarn"); expect(pm.npx).toBe("yarn"); - expect(pm.dlx).toBe("yarn dlx"); + expect(pm.dlx).toEqual(["yarn", "dlx"]); vi.mocked(whichPMRuns).mockReturnValue({ name: "yarn", @@ -170,7 +154,7 @@ describe("Command Helpers", () => { pm = detectPackageManager(); expect(pm.npm).toBe("yarn"); expect(pm.npx).toBe("yarn"); - expect(pm.dlx).toBe("yarn"); + expect(pm.dlx).toEqual(["yarn"]); }); }); diff --git a/packages/create-cloudflare/src/helpers/command.ts b/packages/create-cloudflare/src/helpers/command.ts index c3f68a63ea2..b2633493279 100644 --- a/packages/create-cloudflare/src/helpers/command.ts +++ b/packages/create-cloudflare/src/helpers/command.ts @@ -6,17 +6,12 @@ import { isInteractive, spinner } from "@cloudflare/cli/interactive"; import { spawn } from "cross-spawn"; import { getFrameworkCli } from "frameworks/index"; import { detectPackageManager } from "./packages"; -import * as shellquote from "./shell-quote"; import type { PagesGeneratorContext } from "types"; /** - * Command can be either: - * - a string, like `git commit -m "Changes"` - * - a string array, like ['git', 'commit', '-m', '"Initial commit"'] - * - * The string version is a convenience but is unsafe if your args contain spaces + * Command is a string array, like ['git', 'commit', '-m', '"Initial commit"'] */ -type Command = string | string[]; +type Command = string[]; type RunOptions = { startText?: string; @@ -48,13 +43,9 @@ export const runCommand = async ( command: Command, opts: RunOptions = {} ): Promise => { - if (typeof command === "string") { - command = shellquote.parse(command); - } - return printAsyncStatus({ useSpinner: opts.useSpinner ?? opts.silent, - startText: opts.startText || shellquote.quote(command), + startText: opts.startText || command.join(" "), doneText: opts.doneText, promise() { const [executable, ...args] = command; @@ -188,29 +179,28 @@ export const retry = async ( export const runFrameworkGenerator = async ( ctx: PagesGeneratorContext, - argString: string + args: string[] ) => { const cli = getFrameworkCli(ctx, true); const { npm, dlx } = detectPackageManager(); - // yarn cannot `yarn create@some-version` and doesn't have an npx equivalent // So to retain the ability to lock versions we run it with `npx` and spoof // the user agent so scaffolding tools treat the invocation like yarn - let cmd = `${npm === "yarn" ? "npx" : dlx} ${cli} ${argString}`; + const cmd = [...(npm === "yarn" ? ["npx"] : dlx), cli, ...args]; const env = npm === "yarn" ? { npm_config_user_agent: "yarn" } : {}; if (ctx.framework?.args?.length) { - cmd = `${cmd} ${shellquote.quote(ctx.framework.args)}`; + cmd.push(...ctx.framework.args); } endSection( `Continue with ${ctx.framework?.config.displayName}`, - `via \`${cmd.trim()}\`` + `via \`${cmd.join(" ").trim()}\`` ); if (process.env.VITEST) { const flags = ctx.framework?.config.testFlags ?? []; - cmd = `${cmd} ${shellquote.quote(flags)}`; + cmd.push(...flags); } await runCommand(cmd, { env }); @@ -247,7 +237,7 @@ export const installPackages = async ( break; } - await runCommand(`${npm} ${cmd} ${saveFlag} ${shellquote.quote(packages)}`, { + await runCommand([npm, cmd, saveFlag, ...packages], { ...config, silent: true, }); @@ -271,7 +261,7 @@ export const resetPackageManager = async (ctx: PagesGeneratorContext) => { const lockfilePath = path.join(ctx.project.path, "package-lock.json"); if (existsSync(lockfilePath)) rmSync(lockfilePath); - await runCommand(`${npm} install`, { + await runCommand([npm, "install"], { silent: true, cwd: ctx.project.path, startText: "Installing dependencies", @@ -298,15 +288,7 @@ const needsPackageManagerReset = (ctx: PagesGeneratorContext) => { export const npmInstall = async () => { const { npm } = detectPackageManager(); - const installCmd = [npm, "install"]; - - if (npm === "yarn" && process.env.VITEST) { - // Yarn can corrupt the cache if more than one instance is running at once, - // which is what we do in our tests. - installCmd.push("--mutex", "network"); - } - - await runCommand(installCmd, { + await runCommand([npm, "install"], { silent: true, startText: "Installing dependencies", doneText: `${brandColor("installed")} ${dim(`via \`${npm} install\``)}`, @@ -335,7 +317,7 @@ export const installWrangler = async () => { export const isLoggedIn = async () => { const { npx } = detectPackageManager(); try { - const output = await runCommand(`${npx} wrangler whoami`, { + const output = await runCommand([npx, "wrangler", "whoami"], { silent: true, }); return /You are logged in/.test(output); @@ -357,7 +339,7 @@ export const wranglerLogin = async () => { // We're using a custom spinner since this is a little complicated. // We want to vary the done status based on the output - const output = await runCommand(`${npx} wrangler login`, { + const output = await runCommand([npx, "wrangler", "login"], { silent: true, }); const success = /Successfully logged in/.test(output); @@ -371,7 +353,7 @@ export const wranglerLogin = async () => { export const listAccounts = async () => { const { npx } = detectPackageManager(); - const output = await runCommand(`${npx} wrangler whoami`, { + const output = await runCommand([npx, "wrangler", "whoami"], { silent: true, }); @@ -400,7 +382,7 @@ export const listAccounts = async () => { export async function getWorkerdCompatibilityDate() { const { npm } = detectPackageManager(); - return runCommand(`${npm} info workerd dist-tags.latest`, { + return runCommand([npm, "info", "workerd", "dist-tags.latest"], { silent: true, captureOutput: true, startText: "Retrieving current workerd compatibility date", diff --git a/packages/create-cloudflare/src/helpers/packages.ts b/packages/create-cloudflare/src/helpers/packages.ts index 766996f390a..82842b01b36 100644 --- a/packages/create-cloudflare/src/helpers/packages.ts +++ b/packages/create-cloudflare/src/helpers/packages.ts @@ -47,7 +47,7 @@ export const detectPackageManager = () => { version, npm: "pnpm", npx: "pnpm", - dlx: "pnpm dlx", + dlx: ["pnpm", "dlx"], }; } return { @@ -55,7 +55,7 @@ export const detectPackageManager = () => { version, npm: "pnpm", npx: "pnpx", - dlx: "pnpx", + dlx: ["pnpx"], }; case "yarn": if (semver.gt(version, "2.0.0")) { @@ -64,7 +64,7 @@ export const detectPackageManager = () => { version, npm: "yarn", npx: "yarn", - dlx: "yarn dlx", + dlx: ["yarn", "dlx"], }; } return { @@ -72,7 +72,7 @@ export const detectPackageManager = () => { version, npm: "yarn", npx: "yarn", - dlx: "yarn", + dlx: ["yarn"], }; case "bun": return { @@ -80,7 +80,7 @@ export const detectPackageManager = () => { version, npm: "bun", npx: "bunx", - dlx: "bunx", + dlx: ["bunx"], }; case "npm": @@ -90,7 +90,7 @@ export const detectPackageManager = () => { version, npm: "npm", npx: "npx", - dlx: "npx", + dlx: ["npx"], }; } }; diff --git a/packages/create-cloudflare/src/helpers/shell-quote.ts b/packages/create-cloudflare/src/helpers/shell-quote.ts deleted file mode 100644 index 219ee85ce14..00000000000 --- a/packages/create-cloudflare/src/helpers/shell-quote.ts +++ /dev/null @@ -1,47 +0,0 @@ -import shellquote from "shell-quote"; - -export const quote = function (args: (string | number | boolean)[]) { - const stringArgs = args.map((arg) => String(arg)); - - return shellquote.quote(stringArgs); -}; - -export function parse(cmd: string, env?: Record): string[] { - // This is a workaround for a bug in shell-quote on Windows - // It was fixed and, because it was a breaking change, then reverted - // in https://github.com/ljharb/shell-quote/commit/144e1c2#diff-e727e4b - // We can remove this once we upgrade to a version that includes the fix - // tracked by https://github.com/ljharb/shell-quote/issues/10 - if (process.platform === "win32") { - cmd = cmd.replaceAll("\\", "\\\\"); - } - - const entries = shellquote.parse(cmd, env); - const argv: string[] = []; - - for (const entry of entries) { - // use string entries, as is - if (typeof entry === "string") { - argv.push(entry); - continue; - } - - // ignore comments - if ("comment" in entry) { - continue; - } - - // we don't want to resolve globs, passthrough the pattern unexpanded - if (entry.op === "glob") { - argv.push(entry.pattern); - continue; - } - - // any other entry.op is a ControlOperator (e.g. && or ||) we don't want to support - throw new Error( - `Only simple commands are supported, please don't use the "${entry.op}" operator in "${cmd}".` - ); - } - - return argv; -} diff --git a/packages/create-cloudflare/src/pages.ts b/packages/create-cloudflare/src/pages.ts index b3b3a7f3bcb..452aea05e08 100644 --- a/packages/create-cloudflare/src/pages.ts +++ b/packages/create-cloudflare/src/pages.ts @@ -26,7 +26,6 @@ import { runDeploy, setupProjectDirectory, } from "./common"; -import * as shellquote from "./helpers/shell-quote"; import type { C3Args, PagesGeneratorContext } from "types"; /** How many times to retry the create project command before failing. */ @@ -176,15 +175,21 @@ const createProject = async (ctx: PagesGeneratorContext) => { const CLOUDFLARE_ACCOUNT_ID = ctx.account.id; try { - const compatFlags = shellquote.quote( - ctx.framework?.config.compatibilityFlags ?? [] - ); - const compatFlagsArg = compatFlags - ? `--compatibility-flags ${compatFlags}` - : ""; - + const compatFlags = ctx.framework?.config.compatibilityFlags ?? []; const productionBranch = await getProductionBranch(ctx.project.path); - const cmd = `${npx} wrangler pages project create ${ctx.project.name} --production-branch ${productionBranch} ${compatFlagsArg}`; + const cmd: string[] = [ + npx, + "wrangler", + "pages", + "project", + "create", + ctx.project.name, + "--production-branch", + productionBranch, + ...(compatFlags.length > 0 + ? ["--compatibility-flags", ...compatFlags] + : []), + ]; await retry( { @@ -207,7 +212,9 @@ const createProject = async (ctx: PagesGeneratorContext) => { cwd: ctx.project.path, env: { CLOUDFLARE_ACCOUNT_ID }, startText: "Creating Pages project", - doneText: `${brandColor("created")} ${dim(`via \`${cmd.trim()}\``)}`, + doneText: `${brandColor("created")} ${dim( + `via \`${cmd.join(" ").trim()}\`` + )}`, }) ); } catch (error) { @@ -216,7 +223,15 @@ const createProject = async (ctx: PagesGeneratorContext) => { // Wait until the pages project is available for deployment try { - const verifyProject = `${npx} wrangler pages deployment list --project-name ${ctx.project.name}`; + const verifyProject = [ + npx, + "wrangler", + "pages", + "deployment", + "list", + "--project-name", + ctx.project.name, + ]; await retry({ times: VERIFY_PROJECT_RETRIES }, async () => runCommand(verifyProject, { diff --git a/packages/create-cloudflare/src/workers.ts b/packages/create-cloudflare/src/workers.ts index f6f02d21f33..d9ed472cefa 100644 --- a/packages/create-cloudflare/src/workers.ts +++ b/packages/create-cloudflare/src/workers.ts @@ -123,7 +123,15 @@ async function copyExistingWorkerFiles(ctx: Context) { join(tmpdir(), "c3-wrangler-init--from-dash-") ); await runCommand( - `${dlx} wrangler@3 init --from-dash ${ctx.args.existingScript} -y --no-delegate-c3`, + [ + ...dlx, + "wrangler@3", + "init", + "--from-dash", + ctx.args.existingScript, + "-y", + "--no-delegate-c3", + ], { silent: true, cwd: tempdir, // use a tempdir because we don't want all the files diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3779289b699..aa4b17d5326 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -611,9 +611,6 @@ importers: '@types/semver': specifier: ^7.5.1 version: 7.5.1 - '@types/shell-quote': - specifier: ^1.7.2 - version: 1.7.2 '@types/which-pm-runs': specifier: ^1.0.0 version: 1.0.0 @@ -662,9 +659,6 @@ importers: semver: specifier: ^7.5.1 version: 7.5.1 - shell-quote: - specifier: ^1.8.1 - version: 1.8.1 typescript: specifier: ^5.0.2 version: 5.0.3 From 02a3da6f5e4e6ed8006393044c7de5b7ff807326 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Wed, 15 Nov 2023 10:55:55 +0000 Subject: [PATCH 02/24] C3 e2e - test on CI with Windows + pnpm --- .github/actions/run-c3-e2e/action.yml | 3 ++- .github/workflows/c3-e2e.yml | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/actions/run-c3-e2e/action.yml b/.github/actions/run-c3-e2e/action.yml index 636dbe3c854..7e8932b0727 100644 --- a/.github/actions/run-c3-e2e/action.yml +++ b/.github/actions/run-c3-e2e/action.yml @@ -20,7 +20,8 @@ inputs: runs: using: "composite" steps: - - name: Install Bun ${{ env.bun-version }} + - if: ${{ matrix.pm == 'bun' }} + name: Install Bun ${{ env.bun-version }} uses: oven-sh/setup-bun@v1 with: bun-version: ${{ env.bun-version }} diff --git a/.github/workflows/c3-e2e.yml b/.github/workflows/c3-e2e.yml index dfc4a6fec06..6ab6a469d6d 100644 --- a/.github/workflows/c3-e2e.yml +++ b/.github/workflows/c3-e2e.yml @@ -14,12 +14,16 @@ jobs: concurrency: group: ${{ github.workflow }}-${{ github.ref }}-${{ matrix.os }}-${{ matrix.pm }} cancel-in-progress: true - name: ${{ format('E2E ({0})', matrix.pm) }} + name: ${{ format('E2E ({0} on {1})', matrix.pm, matrix.os) }} if: github.repository_owner == 'cloudflare' && github.event.pull_request.user.login != 'dependabot[bot]' strategy: matrix: os: [ubuntu-latest] pm: [npm, pnpm, bun, yarn] + # include a single windows test with pnpm + include: + - os: windows-latest + pm: pnpm runs-on: ${{ matrix.os }} steps: - name: Checkout Repo From ba452a49332bd886557d485fe0a97e6e23e3e871 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Wed, 15 Nov 2023 16:27:09 +0000 Subject: [PATCH 03/24] C3 e2e - clean up test projects according to their type Previously we only deleted Pages projects - in fact we were trying to delete the Hono (non-Pages) Worker as a Pages project. Now we delete the project based on its type. --- .../e2e-tests/{pages.test.ts => frameworks.test.ts} | 12 ++++++------ packages/create-cloudflare/scripts/common.ts | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) rename packages/create-cloudflare/e2e-tests/{pages.test.ts => frameworks.test.ts} (96%) diff --git a/packages/create-cloudflare/e2e-tests/pages.test.ts b/packages/create-cloudflare/e2e-tests/frameworks.test.ts similarity index 96% rename from packages/create-cloudflare/e2e-tests/pages.test.ts rename to packages/create-cloudflare/e2e-tests/frameworks.test.ts index ffeb9c99d26..cd08b27cf3c 100644 --- a/packages/create-cloudflare/e2e-tests/pages.test.ts +++ b/packages/create-cloudflare/e2e-tests/frameworks.test.ts @@ -10,7 +10,7 @@ import { beforeEach, beforeAll, } from "vitest"; -import { deleteProject } from "../scripts/common"; +import { deleteProject, deleteWorker } from "../scripts/common"; import { frameworkToTest } from "./frameworkToTest"; import { isQuarantineMode, @@ -160,13 +160,13 @@ describe.concurrent(`E2E: Web frameworks`, () => { afterEach(async (ctx) => { const framework = ctx.meta.name; clean(framework); - // Cleanup the pages project in case we need to retry it + // Cleanup the project in case we need to retry it const projectName = getName(framework); - try { + const frameworkConfig = FrameworkMap[framework]; + if (frameworkConfig.type !== "workers") { await deleteProject(projectName); - } catch (error) { - console.error(`Failed to cleanup project: ${projectName}`); - console.error(error); + } else { + await deleteWorker(projectName); } }); diff --git a/packages/create-cloudflare/scripts/common.ts b/packages/create-cloudflare/scripts/common.ts index 14673b56aea..99b822cdd2d 100644 --- a/packages/create-cloudflare/scripts/common.ts +++ b/packages/create-cloudflare/scripts/common.ts @@ -31,7 +31,7 @@ const apiFetch = async ( }); if (response.status >= 400) { - console.error(`REQUEST ERROR: ${url}`); + console.error(`REQUEST ERROR: ${url}`, init); console.error(`(${response.status}) ${response.statusText}`); const body = (await response.json()) as ApiErrorBody; console.error(body.errors); From ed7fd0f922bb570260027c468f04f436912ff7d3 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 16 Nov 2023 10:49:20 +0000 Subject: [PATCH 04/24] C3 - framework template fixes for Windows - do not use bash-style environment variable setting in Docusaurus scripts - do not run TTY interactive e2e tests on Windows --- .../create-cloudflare/e2e-tests/cli.test.ts | 177 +++++++++--------- .../src/frameworks/docusaurus/index.ts | 2 +- 2 files changed, 94 insertions(+), 85 deletions(-) diff --git a/packages/create-cloudflare/e2e-tests/cli.test.ts b/packages/create-cloudflare/e2e-tests/cli.test.ts index fa5389f7497..73571016d70 100644 --- a/packages/create-cloudflare/e2e-tests/cli.test.ts +++ b/packages/create-cloudflare/e2e-tests/cli.test.ts @@ -59,94 +59,103 @@ describe.skipIf(frameworkToTest || isQuarantineMode())( expect(output).toEqual(version); }); - test("Using arrow keys + enter", async (ctx) => { - const { output } = await runC3({ - ctx, - argv: [projectPath], - promptHandlers: [ - { - matcher: /What type of application do you want to create/, - input: [keys.enter], - }, - { - matcher: /Do you want to use TypeScript/, - input: [keys.enter], - }, - { - matcher: /Do you want to use git for version control/, - input: [keys.right, keys.enter], - }, - { - matcher: /Do you want to deploy your application/, - input: [keys.left, keys.enter], - }, - ], - }); + test.skipIf(process.platform === "win32")( + "Using arrow keys + enter", + async (ctx) => { + const { output } = await runC3({ + ctx, + argv: [projectPath], + promptHandlers: [ + { + matcher: /What type of application do you want to create/, + input: [keys.enter], + }, + { + matcher: /Do you want to use TypeScript/, + input: [keys.enter], + }, + { + matcher: /Do you want to use git for version control/, + input: [keys.right, keys.enter], + }, + { + matcher: /Do you want to deploy your application/, + input: [keys.left, keys.enter], + }, + ], + }); - expect(projectPath).toExist(); - expect(output).toContain(`type "Hello World" Worker`); - expect(output).toContain(`yes typescript`); - expect(output).toContain(`no git`); - expect(output).toContain(`no deploy`); - }); + expect(projectPath).toExist(); + expect(output).toContain(`type "Hello World" Worker`); + expect(output).toContain(`yes typescript`); + expect(output).toContain(`no git`); + expect(output).toContain(`no deploy`); + } + ); - test("Typing custom responses", async (ctx) => { - const { output } = await runC3({ - argv: [], - ctx, - promptHandlers: [ - { - matcher: - /In which directory do you want to create your application/, - input: [projectPath, keys.enter], - }, - { - matcher: /What type of application do you want to create/, - input: [keys.down, keys.down, keys.enter], - }, - { - matcher: /Do you want to use TypeScript/, - input: ["n"], - }, - { - matcher: /Do you want to use git for version control/, - input: ["n"], - }, - { - matcher: /Do you want to deploy your application/, - input: ["n"], - }, - ], - }); + test.skipIf(process.platform === "win32")( + "Typing custom responses", + async (ctx) => { + const { output } = await runC3({ + argv: [], + ctx, + promptHandlers: [ + { + matcher: + /In which directory do you want to create your application/, + input: [projectPath, keys.enter], + }, + { + matcher: /What type of application do you want to create/, + input: [keys.down, keys.down, keys.enter], + }, + { + matcher: /Do you want to use TypeScript/, + input: ["n"], + }, + { + matcher: /Do you want to use git for version control/, + input: ["n"], + }, + { + matcher: /Do you want to deploy your application/, + input: ["n"], + }, + ], + }); - expect(projectPath).toExist(); - expect(output).toContain(`type Example router & proxy Worker`); - expect(output).toContain(`no typescript`); - expect(output).toContain(`no git`); - expect(output).toContain(`no deploy`); - }); + expect(projectPath).toExist(); + expect(output).toContain(`type Example router & proxy Worker`); + expect(output).toContain(`no typescript`); + expect(output).toContain(`no git`); + expect(output).toContain(`no deploy`); + } + ); - test("Mixed args and interactive", async (ctx) => { - const { output } = await runC3({ - ctx, - argv: [projectPath, "--ts", "--no-deploy"], - promptHandlers: [ - { - matcher: /What type of application do you want to create/, - input: [keys.enter], - }, - { - matcher: /Do you want to use git for version control/, - input: ["n"], - }, - ], - }); + test.skipIf(process.platform === "win32")( + "Mixed args and interactive", + async (ctx) => { + const { output } = await runC3({ + ctx, + argv: [projectPath, "--ts", "--no-deploy"], + promptHandlers: [ + { + matcher: /What type of application do you want to create/, + input: [keys.enter], + }, + { + matcher: /Do you want to use git for version control/, + input: ["n"], + }, + ], + }); - expect(projectPath).toExist(); - expect(output).toContain(`type "Hello World" Worker`); - expect(output).toContain(`yes typescript`); - expect(output).toContain(`no git`); - expect(output).toContain(`no deploy`); - }); + expect(projectPath).toExist(); + expect(output).toContain(`type "Hello World" Worker`); + expect(output).toContain(`yes typescript`); + expect(output).toContain(`no git`); + expect(output).toContain(`no deploy`); + } + ); } ); diff --git a/packages/create-cloudflare/src/frameworks/docusaurus/index.ts b/packages/create-cloudflare/src/frameworks/docusaurus/index.ts index 29fae9cbf91..2a8df96ce56 100644 --- a/packages/create-cloudflare/src/frameworks/docusaurus/index.ts +++ b/packages/create-cloudflare/src/frameworks/docusaurus/index.ts @@ -14,7 +14,7 @@ const config: FrameworkConfig = { displayName: "Docusaurus", getPackageScripts: async () => ({ "pages:dev": `wrangler pages dev ${await compatDateFlag()} --proxy 3000 -- ${npm} run start`, - "pages:deploy": `NODE_VERSION=16 ${npm} run build && wrangler pages deploy ./build`, + "pages:deploy": `${npm} run build && wrangler pages deploy ./build`, }), testFlags: [`--package-manager`, npm], }; From 1cbc0f71571d1b1e4b00665a7070190454217757 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 16 Nov 2023 11:35:17 +0000 Subject: [PATCH 05/24] C3 e2e - make cleaning up e2e test files more resilient --- packages/create-cloudflare/e2e-tests/helpers.ts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/packages/create-cloudflare/e2e-tests/helpers.ts b/packages/create-cloudflare/e2e-tests/helpers.ts index 96febb014e9..b4e9b7edb67 100644 --- a/packages/create-cloudflare/e2e-tests/helpers.ts +++ b/packages/create-cloudflare/e2e-tests/helpers.ts @@ -1,6 +1,5 @@ import { createWriteStream, - existsSync, mkdirSync, mkdtempSync, realpathSync, @@ -180,12 +179,22 @@ export const testProjectDir = (suite: string) => { const getName = (suffix: string) => `${baseProjectName}-${suffix}`; const getPath = (suffix: string) => join(tmpDirPath, getName(suffix)); const clean = (suffix: string) => { - const path = getPath(suffix); - if (existsSync(path)) { + try { + const path = getPath(suffix); rmSync(path, { recursive: true, force: true, + maxRetries: 10, + retryDelay: 100, }); + } catch (e) { + if (typeof e === "object" && e !== null && "code" in e) { + const code = e.code; + if (code === "EBUSY" || code === "ENOENT") { + return; + } + } + throw e; } }; From ed9761a3368654b8b1837f42ad4f6c5131ed3ea5 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 16 Nov 2023 12:12:01 +0000 Subject: [PATCH 06/24] C3 - update Nuxt template to avoid requiring Windows incompatible shell environment variable --- .changeset/popular-dolls-enjoy.md | 10 +++++++++ .../e2e-tests/frameworks.test.ts | 5 ----- .../src/frameworks/nuxt/index.ts | 21 ++++++++++++++++++- 3 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 .changeset/popular-dolls-enjoy.md diff --git a/.changeset/popular-dolls-enjoy.md b/.changeset/popular-dolls-enjoy.md new file mode 100644 index 00000000000..edb50a28129 --- /dev/null +++ b/.changeset/popular-dolls-enjoy.md @@ -0,0 +1,10 @@ +--- +"create-cloudflare": patch +--- + +fix: update Nuxt template to work on Windows + +Rather than relying upon the non-Windows shell syntax to specify an environment variable, +we now update the `nuxt.config.ts` files to include the cloudflare preset. + +Fixes #4285 diff --git a/packages/create-cloudflare/e2e-tests/frameworks.test.ts b/packages/create-cloudflare/e2e-tests/frameworks.test.ts index cd08b27cf3c..2d2a81279c9 100644 --- a/packages/create-cloudflare/e2e-tests/frameworks.test.ts +++ b/packages/create-cloudflare/e2e-tests/frameworks.test.ts @@ -93,11 +93,6 @@ describe.concurrent(`E2E: Web frameworks`, () => { }, nuxt: { expectResponseToContain: "Welcome to Nuxt!", - overrides: { - packageScripts: { - build: "NITRO_PRESET=cloudflare-pages nuxt build", - }, - }, testCommitMessage: true, }, react: { diff --git a/packages/create-cloudflare/src/frameworks/nuxt/index.ts b/packages/create-cloudflare/src/frameworks/nuxt/index.ts index 377c0636238..55fbb3d7d22 100644 --- a/packages/create-cloudflare/src/frameworks/nuxt/index.ts +++ b/packages/create-cloudflare/src/frameworks/nuxt/index.ts @@ -1,4 +1,8 @@ +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; import { logRaw } from "@cloudflare/cli"; +import { brandColor, dim } from "@cloudflare/cli/colors"; +import { spinner } from "@cloudflare/cli/interactive"; import { npmInstall, runFrameworkGenerator } from "helpers/command"; import { compatDateFlag, writeFile } from "helpers/files"; import { detectPackageManager } from "helpers/packages"; @@ -23,6 +27,7 @@ const generate = async (ctx: PagesGeneratorContext) => { const configure = async (ctx: PagesGeneratorContext) => { process.chdir(ctx.project.path); writeFile("./.node-version", "17"); + await updateNuxtConfig(); await npmInstall(); }; @@ -31,9 +36,23 @@ const config: FrameworkConfig = { configure, displayName: "Nuxt", getPackageScripts: async () => ({ - build: (cmd) => `NITRO_PRESET=cloudflare-pages ${cmd}`, "pages:dev": `wrangler pages dev ${await compatDateFlag()} --proxy 3000 -- ${npm} run dev`, "pages:deploy": `${npm} run build && wrangler pages deploy ./dist`, }), }; export default config; + +function updateNuxtConfig() { + const configFileName = "nuxt.config.ts"; + const configFilePath = resolve(configFileName); + const s = spinner(); + s.start(`Updating \`${configFileName}\``); + // Add the cloudflare preset into the configuration file. + const originalConfigFile = readFileSync(configFilePath, "utf8"); + const updatedConfigFile = originalConfigFile.replace( + "defineNuxtConfig({", + "defineNuxtConfig({\n nitro: {\n preset: 'cloudflare-pages'\n }," + ); + writeFile(configFilePath, updatedConfigFile); + s.stop(`${brandColor(`updated`)} ${dim(`\`${configFileName}\``)}`); +} From 92f02cc9846cbb9900c0296c3a7f04458d559f67 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 16 Nov 2023 15:27:21 +0000 Subject: [PATCH 07/24] C3 e2e - remove noise from expected test errors --- packages/create-cloudflare/scripts/common.ts | 77 +++++++++++--------- 1 file changed, 41 insertions(+), 36 deletions(-) diff --git a/packages/create-cloudflare/scripts/common.ts b/packages/create-cloudflare/scripts/common.ts index 99b822cdd2d..81181560a32 100644 --- a/packages/create-cloudflare/scripts/common.ts +++ b/packages/create-cloudflare/scripts/common.ts @@ -31,16 +31,7 @@ const apiFetch = async ( }); if (response.status >= 400) { - console.error(`REQUEST ERROR: ${url}`, init); - console.error(`(${response.status}) ${response.statusText}`); - const body = (await response.json()) as ApiErrorBody; - console.error(body.errors); - - // Returning null instead of throwing an error here allows the caller to decide whether - // to continue on fail or not. A failure to list projects should end the script, whereas - // a failure to delete a project may happen due to concurrent runs of this script, and should - // be tolerated. - return null; + throw { url, init, response }; } const json = (await response.json()) as ApiSuccessBody; @@ -54,52 +45,66 @@ export const listC3Projects = async () => { const projects = []; while (projects.length % pageSize === 0) { - const res = await apiFetch( - `/pages/projects`, - { method: "GET" }, - { - per_page: pageSize, - page, + try { + const res = await apiFetch( + `/pages/projects`, + { method: "GET" }, + { + per_page: pageSize, + page, + } + ); + projects.push(...res); + page++; + if (res.length < pageSize) { + break; } - ); - - if (res === null) { + } catch (e) { + const { url, init, response } = e as any; console.error("Failed to fetch project list"); + console.error(url, init); + console.error(`(${response.status}) ${response.statusText}`); + const body = (await response.json()) as ApiErrorBody; + console.error(body.errors); process.exit(1); } - - projects.push(...res); - page++; - if (res.length < pageSize) { - break; - } } return projects.filter((p) => p.name.startsWith("c3-e2e-")); }; export const deleteProject = async (project: string) => { - await apiFetch(`/pages/projects/${project}`, { - method: "DELETE", - }); + try { + await apiFetch(`/pages/projects/${project}`, { + method: "DELETE", + }); + } catch { + // Ignore errors + } }; export const listC3Workers = async () => { const pageSize = 10; let page = 1; - const res = await apiFetch(`/workers/scripts`, { method: "GET" }); - - if (res === null) { + try { + const res = await apiFetch(`/workers/scripts`, { method: "GET" }); + return res.filter((p) => p.id.startsWith("c3-e2e-")); + } catch (e) { + const { url, init, response } = e as any; console.error("Failed to fetch workers list"); + console.error(url, init); + console.error(`(${response.status}) ${response.statusText}`); + const body = (await response.json()) as ApiErrorBody; + console.error(body.errors); process.exit(1); } - - return res.filter((p) => p.id.startsWith("c3-e2e-")); }; export const deleteWorker = async (id: string) => { - await apiFetch(`/workers/scripts/${id}`, { - method: "DELETE", - }); + try { + await apiFetch(`/workers/scripts/${id}`, { + method: "DELETE", + }); + } catch {} }; From 13c6c4e317845af3ea892445141865a577624c31 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Mon, 20 Nov 2023 14:55:22 +0000 Subject: [PATCH 08/24] C3 e2e - Only cleanup workers and projects that are over 1 hour old These e2e test workers and projects should be cleaned up as part of the normal test completion. But if the test crashes they may be left orphaned. This change ensures that we do not clean up projects to early while they are still being used. --- packages/create-cloudflare/scripts/common.ts | 27 ++++++++++++++----- .../create-cloudflare/scripts/e2eCleanup.ts | 2 +- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/packages/create-cloudflare/scripts/common.ts b/packages/create-cloudflare/scripts/common.ts index 81181560a32..309b118c5ed 100644 --- a/packages/create-cloudflare/scripts/common.ts +++ b/packages/create-cloudflare/scripts/common.ts @@ -10,6 +10,12 @@ type ApiSuccessBody = { export type Project = { name: string; + created_on: string; +}; + +export type Worker = { + id: string; + created_on: string; }; const apiFetch = async ( @@ -43,7 +49,7 @@ export const listC3Projects = async () => { const pageSize = 10; let page = 1; - const projects = []; + const projects: Project[] = []; while (projects.length % pageSize === 0) { try { const res = await apiFetch( @@ -70,7 +76,12 @@ export const listC3Projects = async () => { } } - return projects.filter((p) => p.name.startsWith("c3-e2e-")); + return projects.filter( + (p) => + p.name.startsWith("c3-e2e-") && + // Projects are more than an hour old + Date.now() - new Date(p.created_on).valueOf() > 1000 * 60 * 60 + ); }; export const deleteProject = async (project: string) => { @@ -84,12 +95,14 @@ export const deleteProject = async (project: string) => { }; export const listC3Workers = async () => { - const pageSize = 10; - let page = 1; - try { - const res = await apiFetch(`/workers/scripts`, { method: "GET" }); - return res.filter((p) => p.id.startsWith("c3-e2e-")); + const res: Worker[] = await apiFetch(`/workers/scripts`, { method: "GET" }); + return res.filter( + (p) => + p.id.startsWith("c3-e2e-") && + // Workers are more than an hour old + Date.now() - new Date(p.created_on).valueOf() > 1000 * 60 * 60 + ); } catch (e) { const { url, init, response } = e as any; console.error("Failed to fetch workers list"); diff --git a/packages/create-cloudflare/scripts/e2eCleanup.ts b/packages/create-cloudflare/scripts/e2eCleanup.ts index dfaeeb63145..c5e285e979c 100644 --- a/packages/create-cloudflare/scripts/e2eCleanup.ts +++ b/packages/create-cloudflare/scripts/e2eCleanup.ts @@ -17,7 +17,7 @@ if (!process.env.CLOUDFLARE_ACCOUNT_ID) { } const run = async () => { - const projectsToDelete = (await listC3Projects()) as Project[]; + const projectsToDelete = await listC3Projects(); for (const project of projectsToDelete) { console.log("Deleting Pages project: " + project.name); From 898b2e3503acad038fe0d98995c349e3367096c3 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Mon, 20 Nov 2023 17:12:24 +0000 Subject: [PATCH 09/24] C3 e2e - only clean up test projects once per day at 3am --- .github/workflows/c3-e2e-project-cleanup.yml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/.github/workflows/c3-e2e-project-cleanup.yml b/.github/workflows/c3-e2e-project-cleanup.yml index 43573a6d770..d699f11198a 100644 --- a/.github/workflows/c3-e2e-project-cleanup.yml +++ b/.github/workflows/c3-e2e-project-cleanup.yml @@ -2,10 +2,8 @@ name: C3 E2E Project Cleanup on: - workflow_run: - workflows: [C3 E2E Tests, C3 E2E Tests (Dependabot), C3 E2E (Quarantine)] - types: - - completed + schedule: + - cron: "0 3 * * *" # Run at 3am each day env: node-version: 18.17.1 jobs: From e36692929e1a32291b2328a4e01b7c0794b0957a Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 16 Nov 2023 15:27:53 +0000 Subject: [PATCH 10/24] C3 e2e - fix commit message tests on Windows --- packages/create-cloudflare/e2e-tests/helpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/create-cloudflare/e2e-tests/helpers.ts b/packages/create-cloudflare/e2e-tests/helpers.ts index b4e9b7edb67..21405c395dd 100644 --- a/packages/create-cloudflare/e2e-tests/helpers.ts +++ b/packages/create-cloudflare/e2e-tests/helpers.ts @@ -235,7 +235,7 @@ export const testDeploymentCommitMessage = async ( (project) => project.name === projectName )?.latest_deployment?.deployment_trigger?.metadata?.commit_message; expect(projectLatestCommitMessage).toMatch( - /^Initialize web application via create-cloudflare CLI/ + /Initialize web application via create-cloudflare CLI/ ); expect(projectLatestCommitMessage).toContain( `C3 = create-cloudflare@${version}` From 4725c5cc2827dcbe39d522e9668fcff78a980901 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 16 Nov 2023 15:28:15 +0000 Subject: [PATCH 11/24] C3 e2e - recreate test folders for each e2e retry Previously we were reusing folders after clearing them when retrying a failed test. But this could lead to problems, especially on Windows where clearing out the folder did not always work. --- .../e2e-tests/frameworks.test.ts | 67 +++++++++---------- 1 file changed, 30 insertions(+), 37 deletions(-) diff --git a/packages/create-cloudflare/e2e-tests/frameworks.test.ts b/packages/create-cloudflare/e2e-tests/frameworks.test.ts index 2d2a81279c9..050d8b6282e 100644 --- a/packages/create-cloudflare/e2e-tests/frameworks.test.ts +++ b/packages/create-cloudflare/e2e-tests/frameworks.test.ts @@ -2,14 +2,7 @@ import { join } from "path"; import { FrameworkMap } from "frameworks/index"; import { readJSON } from "helpers/files"; import { fetch } from "undici"; -import { - describe, - expect, - test, - afterEach, - beforeEach, - beforeAll, -} from "vitest"; +import { describe, expect, test, beforeAll } from "vitest"; import { deleteProject, deleteWorker } from "../scripts/common"; import { frameworkToTest } from "./frameworkToTest"; import { @@ -141,36 +134,15 @@ describe.concurrent(`E2E: Web frameworks`, () => { }, }; - const { getPath, getName, clean } = testProjectDir("pages"); - beforeAll((ctx) => { recreateLogFolder(ctx as Suite); }); - beforeEach((ctx) => { - const framework = ctx.meta.name; - clean(framework); - }); - - afterEach(async (ctx) => { - const framework = ctx.meta.name; - clean(framework); - // Cleanup the project in case we need to retry it - const projectName = getName(framework); - const frameworkConfig = FrameworkMap[framework]; - if (frameworkConfig.type !== "workers") { - await deleteProject(projectName); - } else { - await deleteWorker(projectName); - } - }); - const runCli = async ( framework: string, + projectPath: string, { ctx, argv = [], promptHandlers = [], overrides }: RunnerConfig ) => { - const projectPath = getPath(framework); - const args = [ projectPath, "--type", @@ -224,13 +196,15 @@ describe.concurrent(`E2E: Web frameworks`, () => { const runCliWithDeploy = async ( framework: string, + projectName: string, + projectPath: string, ctx: TestContext, testCommitMessage: boolean ) => { const { argv, overrides, promptHandlers, expectResponseToContain } = frameworkTests[framework]; - const { output } = await runCli(framework, { + const { output } = await runCli(framework, projectPath, { ctx, overrides, promptHandlers, @@ -259,7 +233,7 @@ describe.concurrent(`E2E: Web frameworks`, () => { ).toContain(expectResponseToContain); if (testCommitMessage) { - await testDeploymentCommitMessage(getName(framework), framework); + await testDeploymentCommitMessage(projectName, framework); } }; @@ -278,17 +252,36 @@ describe.concurrent(`E2E: Web frameworks`, () => { // Skip if the package manager is unsupported shouldRun &&= !unsupportedPms?.includes(process.env.TEST_PM ?? ""); - test.runIf(shouldRun)( framework, async (ctx) => { - await runCliWithDeploy(framework, ctx, testCommitMessage); + const { getPath, getName, clean } = testProjectDir("pages"); + const projectPath = getPath(framework); + const projectName = getName(framework); + const frameworkConfig = FrameworkMap[framework]; + try { + await runCliWithDeploy( + framework, + projectName, + projectPath, + ctx, + testCommitMessage + ); + } finally { + clean(framework); + // Cleanup the project in case we need to retry it + if (frameworkConfig.type !== "workers") { + await deleteProject(projectName); + } else { + await deleteWorker(projectName); + } + } }, { retry: 3, timeout: timeout || TEST_TIMEOUT } ); }); - test.skip("Hono (wrangler defaults)", async (ctx) => { - await runCli("hono", { ctx, argv: ["--wrangler-defaults"] }); - }); + // test.skip("Hono (wrangler defaults)", async (ctx) => { + // await runCli("hono", { ctx, argv: ["--wrangler-defaults"] }); + // }); }); From adf202212b2675a62b0d0098f46c97aa064f8112 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 16 Nov 2023 16:29:18 +0000 Subject: [PATCH 12/24] C3 e2e - bump test timeout longer --- .github/workflows/c3-e2e.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/c3-e2e.yml b/.github/workflows/c3-e2e.yml index 6ab6a469d6d..bc2d35a1304 100644 --- a/.github/workflows/c3-e2e.yml +++ b/.github/workflows/c3-e2e.yml @@ -10,7 +10,7 @@ env: bun-version: 1.0.3 jobs: e2e: - timeout-minutes: 30 + timeout-minutes: 45 concurrency: group: ${{ github.workflow }}-${{ github.ref }}-${{ matrix.os }}-${{ matrix.pm }} cancel-in-progress: true From 1af80264c717275ed037a3b56cce6ecadf75437a Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 21 Nov 2023 15:09:25 +0000 Subject: [PATCH 13/24] C3 e2e - do not run C3 e2e tests on unsupported OSes --- .../e2e-tests/frameworks.test.ts | 30 ++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/packages/create-cloudflare/e2e-tests/frameworks.test.ts b/packages/create-cloudflare/e2e-tests/frameworks.test.ts index 050d8b6282e..712fe317807 100644 --- a/packages/create-cloudflare/e2e-tests/frameworks.test.ts +++ b/packages/create-cloudflare/e2e-tests/frameworks.test.ts @@ -16,13 +16,15 @@ import { import type { RunnerConfig } from "./helpers"; import type { Suite, TestContext } from "vitest"; -const TEST_TIMEOUT = 1000 * 60 * 3; +const TEST_TIMEOUT = 1000 * 60 * 5; +const LONG_TIMEOUT = 1000 * 60 * 6; type FrameworkTestConfig = Omit & { expectResponseToContain: string; testCommitMessage: boolean; timeout?: number; unsupportedPms?: string[]; + unsupportedOSs?: string[]; }; describe.concurrent(`E2E: Web frameworks`, () => { @@ -31,15 +33,17 @@ describe.concurrent(`E2E: Web frameworks`, () => { astro: { expectResponseToContain: "Hello, Astronaut!", testCommitMessage: true, + unsupportedOSs: ["win32"], }, docusaurus: { expectResponseToContain: "Dinosaurs are cool", unsupportedPms: ["bun"], testCommitMessage: true, - timeout: 1000 * 60 * 5, + timeout: LONG_TIMEOUT, }, angular: { expectResponseToContain: "Congratulations! Your app is running.", + unsupportedOSs: ["win32"], testCommitMessage: true, }, gatsby: { @@ -52,7 +56,7 @@ describe.concurrent(`E2E: Web frameworks`, () => { }, ], testCommitMessage: true, - timeout: 1000 * 60 * 6, + timeout: LONG_TIMEOUT, }, hono: { expectResponseToContain: "Hello Hono!", @@ -67,11 +71,12 @@ describe.concurrent(`E2E: Web frameworks`, () => { }, ], testCommitMessage: true, + unsupportedOSs: ["win32"], }, remix: { expectResponseToContain: "Welcome to Remix", testCommitMessage: true, - timeout: 1000 * 60 * 5, + timeout: LONG_TIMEOUT, }, next: { expectResponseToContain: "Create Next App", @@ -87,10 +92,12 @@ describe.concurrent(`E2E: Web frameworks`, () => { nuxt: { expectResponseToContain: "Welcome to Nuxt!", testCommitMessage: true, + timeout: LONG_TIMEOUT, }, react: { expectResponseToContain: "React App", testCommitMessage: true, + timeout: LONG_TIMEOUT, }, solid: { expectResponseToContain: "Hello world", @@ -109,6 +116,7 @@ describe.concurrent(`E2E: Web frameworks`, () => { }, ], testCommitMessage: true, + timeout: LONG_TIMEOUT, }, svelte: { expectResponseToContain: "SvelteKit app", @@ -127,6 +135,7 @@ describe.concurrent(`E2E: Web frameworks`, () => { }, ], testCommitMessage: true, + unsupportedOSs: ["win32"], }, vue: { expectResponseToContain: "Vite App", @@ -238,8 +247,13 @@ describe.concurrent(`E2E: Web frameworks`, () => { }; Object.keys(frameworkTests).forEach((framework) => { - const { quarantine, timeout, testCommitMessage, unsupportedPms } = - frameworkTests[framework]; + const { + quarantine, + timeout, + testCommitMessage, + unsupportedPms, + unsupportedOSs, + } = frameworkTests[framework]; const quarantineModeMatch = isQuarantineMode() == (quarantine ?? false); @@ -252,6 +266,8 @@ describe.concurrent(`E2E: Web frameworks`, () => { // Skip if the package manager is unsupported shouldRun &&= !unsupportedPms?.includes(process.env.TEST_PM ?? ""); + // Skip if the OS is unsupported + shouldRun &&= !unsupportedOSs?.includes(process.platform); test.runIf(shouldRun)( framework, async (ctx) => { @@ -277,7 +293,7 @@ describe.concurrent(`E2E: Web frameworks`, () => { } } }, - { retry: 3, timeout: timeout || TEST_TIMEOUT } + { retry: 1, timeout: timeout || TEST_TIMEOUT } ); }); From 6e8dcb8bea25fb9c0496c61242ef185e62fee837 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Mon, 20 Nov 2023 13:44:43 +0000 Subject: [PATCH 14/24] C3 e2e - store e2e logs separately by OS --- .github/actions/run-c3-e2e/action.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/actions/run-c3-e2e/action.yml b/.github/actions/run-c3-e2e/action.yml index 7e8932b0727..b9f72354098 100644 --- a/.github/actions/run-c3-e2e/action.yml +++ b/.github/actions/run-c3-e2e/action.yml @@ -47,7 +47,7 @@ runs: - name: Upload Logs uses: actions/upload-artifact@v3 with: - name: e2e-logs + name: e2e-logs-${{matrix.os}} path: packages/create-cloudflare/.e2e-logs - name: Fail if errors detected From f77d164cd4b810d67234664d39d4848f6bffd2af Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Mon, 20 Nov 2023 14:55:49 +0000 Subject: [PATCH 15/24] C3 e2e - Retry some of the checks a few times to reduce flakiness --- .../e2e-tests/frameworks.test.ts | 19 +++--- .../create-cloudflare/e2e-tests/helpers.ts | 59 +++++++++++-------- 2 files changed, 45 insertions(+), 33 deletions(-) diff --git a/packages/create-cloudflare/e2e-tests/frameworks.test.ts b/packages/create-cloudflare/e2e-tests/frameworks.test.ts index 712fe317807..62703dfaa67 100644 --- a/packages/create-cloudflare/e2e-tests/frameworks.test.ts +++ b/packages/create-cloudflare/e2e-tests/frameworks.test.ts @@ -1,5 +1,6 @@ import { join } from "path"; import { FrameworkMap } from "frameworks/index"; +import { retry } from "helpers/command"; import { readJSON } from "helpers/files"; import { fetch } from "undici"; import { describe, expect, test, beforeAll } from "vitest"; @@ -232,14 +233,16 @@ describe.concurrent(`E2E: Web frameworks`, () => { const projectUrl = match[1]; - const res = await fetch(projectUrl); - expect(res.status).toBe(200); - - const body = await res.text(); - expect( - body, - `(${framework}) Deployed page (${projectUrl}) didn't contain expected string: "${expectResponseToContain}"` - ).toContain(expectResponseToContain); + await retry({ times: 5 }, async () => { + await new Promise((resolve) => setTimeout(resolve, 1000)); // wait a second + const res = await fetch(projectUrl); + const body = await res.text(); + if (!body.includes(expectResponseToContain)) { + throw new Error( + `(${framework}) Deployed page (${projectUrl}) didn't contain expected string: "${expectResponseToContain}"` + ); + } + }); if (testCommitMessage) { await testDeploymentCommitMessage(projectName, framework); diff --git a/packages/create-cloudflare/e2e-tests/helpers.ts b/packages/create-cloudflare/e2e-tests/helpers.ts index 21405c395dd..4d9f6acc13b 100644 --- a/packages/create-cloudflare/e2e-tests/helpers.ts +++ b/packages/create-cloudflare/e2e-tests/helpers.ts @@ -10,6 +10,7 @@ import { tmpdir } from "os"; import { basename, join } from "path"; import { stripAnsi } from "@cloudflare/cli"; import { spawn } from "cross-spawn"; +import { retry } from "helpers/command"; import { sleep } from "helpers/common"; import { detectPackageManager } from "helpers/packages"; import { fetch } from "undici"; @@ -205,35 +206,43 @@ export const testDeploymentCommitMessage = async ( projectName: string, framework: string ) => { - // Note: we cannot simply run git and check the result since the commit can be part of the - // deployment even without git, so instead we fetch the deployment info from the pages api - const response = await fetch( - `https://api.cloudflare.com/client/v4/accounts/${process.env.CLOUDFLARE_ACCOUNT_ID}/pages/projects`, - { - headers: { - Authorization: `Bearer ${process.env.CLOUDFLARE_API_TOKEN}`, - }, - } - ); - - const result = ( - (await response.json()) as { - result: { - name: string; - latest_deployment?: { - deployment_trigger: { - metadata?: { - commit_message: string; + const projectLatestCommitMessage = await retry({ times: 5 }, async () => { + // Wait for 2 seconds between each attempt + await new Promise((resolve) => setTimeout(resolve, 2000)); + // Note: we cannot simply run git and check the result since the commit can be part of the + // deployment even without git, so instead we fetch the deployment info from the pages api + const response = await fetch( + `https://api.cloudflare.com/client/v4/accounts/${process.env.CLOUDFLARE_ACCOUNT_ID}/pages/projects`, + { + headers: { + Authorization: `Bearer ${process.env.CLOUDFLARE_API_TOKEN}`, + }, + } + ); + + const result = ( + (await response.json()) as { + result: { + name: string; + latest_deployment?: { + deployment_trigger: { + metadata?: { + commit_message: string; + }; }; }; - }; - }[]; + }[]; + } + ).result; + + const commitMessage = result.find((project) => project.name === projectName) + ?.latest_deployment?.deployment_trigger?.metadata?.commit_message; + if (!commitMessage) { + throw new Error("Could not find deployment with name " + projectName); } - ).result; + return commitMessage; + }); - const projectLatestCommitMessage = result.find( - (project) => project.name === projectName - )?.latest_deployment?.deployment_trigger?.metadata?.commit_message; expect(projectLatestCommitMessage).toMatch( /Initialize web application via create-cloudflare CLI/ ); From a77cbedd61f0675c8a98d7d380973cd754f3595b Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Mon, 20 Nov 2023 20:42:52 +0000 Subject: [PATCH 16/24] C3 e2e - Record vitest results as a json artifact for downloading --- packages/create-cloudflare/vitest-e2e.config.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/create-cloudflare/vitest-e2e.config.ts b/packages/create-cloudflare/vitest-e2e.config.ts index e032a49ec63..43d1ab33ce7 100644 --- a/packages/create-cloudflare/vitest-e2e.config.ts +++ b/packages/create-cloudflare/vitest-e2e.config.ts @@ -7,8 +7,12 @@ export default defineConfig({ include: ["e2e-tests/**/*.test.ts"], cache: false, root: ".", - testTimeout: 1000 * 60 * 5, // 5 min for lengthy installs + testTimeout: 1000 * 60 * 10, // 10 min for lengthy installs maxConcurrency: 3, setupFiles: ["e2e-tests/setup.ts"], + reporters: ["json", "verbose", "hanging-process"], + outputFile: { + json: "./.e2e-logs/" + process.env.TEST_PM + "/results.json", + }, }, }); From 0dcb6cf51600de6ec23f411ddacc8beccc1f1d9a Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 21 Nov 2023 10:43:29 +0000 Subject: [PATCH 17/24] C3 e2e - disable global caches for package managers This commit uses environment variables to tell package managers to put cached files in a local directory rather than a global shared one, which can cause problems with race conditions when running multiple installs at the same time. --- packages/create-cloudflare/e2e-tests/helpers.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/create-cloudflare/e2e-tests/helpers.ts b/packages/create-cloudflare/e2e-tests/helpers.ts index 4d9f6acc13b..419b97bb1b9 100644 --- a/packages/create-cloudflare/e2e-tests/helpers.ts +++ b/packages/create-cloudflare/e2e-tests/helpers.ts @@ -53,7 +53,17 @@ export const runC3 = async ({ }: RunnerConfig) => { const cmd = "node"; const args = ["./dist/cli.js", ...argv]; - const proc = spawn(cmd, args); + const proc = spawn(cmd, args, { + env: { + ...process.env, + // The following env vars are set to ensure that package managers + // do not use the same global cache and accidentally hit race conditions. + YARN_CACHE_FOLDER: "./.yarn/cache", + YARN_ENABLE_GLOBAL_CACHE: "false", + PNPM_HOME: "./.pnpm", + npm_config_cache: "./.npm/cache", + }, + }); promptHandlers = [...promptHandlers]; From e658af7863f547a6127934cff45afb5477c08999 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 21 Nov 2023 11:24:17 +0000 Subject: [PATCH 18/24] C3 e2e - don't cancel other matrix jobs if one fails It is a false optimization to cancel jobs that are likely to pass when another job flakes out. --- .github/workflows/c3-e2e.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/c3-e2e.yml b/.github/workflows/c3-e2e.yml index bc2d35a1304..8f65a8b31e1 100644 --- a/.github/workflows/c3-e2e.yml +++ b/.github/workflows/c3-e2e.yml @@ -17,6 +17,7 @@ jobs: name: ${{ format('E2E ({0} on {1})', matrix.pm, matrix.os) }} if: github.repository_owner == 'cloudflare' && github.event.pull_request.user.login != 'dependabot[bot]' strategy: + fail-fast: false matrix: os: [ubuntu-latest] pm: [npm, pnpm, bun, yarn] From 5d322d4c12ce27d7bcd079297dc48fe0f8b87186 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 21 Nov 2023 15:49:51 +0000 Subject: [PATCH 19/24] ci: retry the external-durable-objects-app fixture if it flakes --- .../tests/index.test.ts | 178 ++++++++++-------- 1 file changed, 95 insertions(+), 83 deletions(-) diff --git a/fixtures/external-durable-objects-app/tests/index.test.ts b/fixtures/external-durable-objects-app/tests/index.test.ts index cde97697fdf..b43b7f4be95 100644 --- a/fixtures/external-durable-objects-app/tests/index.test.ts +++ b/fixtures/external-durable-objects-app/tests/index.test.ts @@ -5,99 +5,111 @@ import { describe, expect, it, beforeAll, afterAll } from "vitest"; import type { ChildProcess } from "child_process"; import { type UnstableDevWorker, unstable_dev } from "wrangler"; -describe.skip("Pages Functions", () => { - let a: UnstableDevWorker; - let b: UnstableDevWorker; - let c: UnstableDevWorker; +describe.skip( + "Pages Functions", + () => { + let a: UnstableDevWorker; + let b: UnstableDevWorker; + let c: UnstableDevWorker; - let dWranglerProcess: ChildProcess; - let dIP: string; - let dPort: number; - let dResolveReadyPromise: (value: unknown) => void; - const dReadyPromise = new Promise((resolve) => { - dResolveReadyPromise = resolve; - }); - - beforeAll(async () => { - a = await unstable_dev(path.join(__dirname, "../a/index.ts"), { - config: path.join(__dirname, "../a/wrangler.toml"), - }); - b = await unstable_dev(path.join(__dirname, "../b/index.ts"), { - config: path.join(__dirname, "../b/wrangler.toml"), + let dWranglerProcess: ChildProcess; + let dIP: string; + let dPort: number; + let dResolveReadyPromise: (value: unknown) => void; + const dReadyPromise = new Promise((resolve) => { + dResolveReadyPromise = resolve; }); - c = await unstable_dev(path.join(__dirname, "../c/index.ts"), { - config: path.join(__dirname, "../c/wrangler.toml"), - }); + beforeAll(async () => { + a = await unstable_dev(path.join(__dirname, "../a/index.ts"), { + config: path.join(__dirname, "../a/wrangler.toml"), + }); + b = await unstable_dev(path.join(__dirname, "../b/index.ts"), { + config: path.join(__dirname, "../b/wrangler.toml"), + }); - dWranglerProcess = fork( - path.join("..", "..", "..", "packages", "wrangler", "bin", "wrangler.js"), - [ - "pages", - "dev", - "public", - "--do=PAGES_REFERENCED_DO=MyDurableObject@a", - "--port=0", - "--inspector-port=0", - ], - { - stdio: ["ignore", "ignore", "ignore", "ipc"], - cwd: path.resolve(__dirname, "..", "d"), - } - ).on("message", (message) => { - const parsedMessage = JSON.parse(message.toString()); - dIP = parsedMessage.ip; - dPort = parsedMessage.port; - dResolveReadyPromise(undefined); + c = await unstable_dev(path.join(__dirname, "../c/index.ts"), { + config: path.join(__dirname, "../c/wrangler.toml"), + }); + + dWranglerProcess = fork( + path.join( + "..", + "..", + "..", + "packages", + "wrangler", + "bin", + "wrangler.js" + ), + [ + "pages", + "dev", + "public", + "--do=PAGES_REFERENCED_DO=MyDurableObject@a", + "--port=0", + "--inspector-port=0", + ], + { + stdio: ["ignore", "ignore", "ignore", "ipc"], + cwd: path.resolve(__dirname, "..", "d"), + } + ).on("message", (message) => { + const parsedMessage = JSON.parse(message.toString()); + dIP = parsedMessage.ip; + dPort = parsedMessage.port; + dResolveReadyPromise(undefined); + }); }); - }); - afterAll(async () => { - await dReadyPromise; - await a.stop(); - await b.stop(); - await c.stop(); + afterAll(async () => { + await dReadyPromise; + await a.stop(); + await b.stop(); + await c.stop(); - await new Promise((resolve, reject) => { - dWranglerProcess.once("exit", (code) => { - if (!code) { - resolve(code); - } else { - reject(code); - } + await new Promise((resolve, reject) => { + dWranglerProcess.once("exit", (code) => { + if (!code) { + resolve(code); + } else { + reject(code); + } + }); + dWranglerProcess.kill("SIGTERM"); }); - dWranglerProcess.kill("SIGTERM"); }); - }); - it("connects up Durable Objects and keeps state across wrangler instances", async () => { - await dReadyPromise; + it("connects up Durable Objects and keeps state across wrangler instances", async () => { + await dReadyPromise; - // Service registry is polled every 300ms, - // so let's give all the Workers a little time to find each other - await new Promise((resolve) => setTimeout(resolve, 700)); + // Service registry is polled every 300ms, + // so let's give all the Workers a little time to find each other + await new Promise((resolve) => setTimeout(resolve, 700)); - const responseA = await a.fetch(`/`, { - headers: { - "X-Reset-Count": "true", - }, + const responseA = await a.fetch(`/`, { + headers: { + "X-Reset-Count": "true", + }, + }); + const dataA = (await responseA.json()) as { count: number; id: string }; + expect(dataA.count).toEqual(1); + const responseB = await b.fetch(`/`); + const dataB = (await responseB.json()) as { count: number; id: string }; + expect(dataB.count).toEqual(2); + const responseC = await c.fetch(`/`); + const dataC = (await responseC.json()) as { count: number; id: string }; + expect(dataC.count).toEqual(3); + const responseD = await fetch(`http://${dIP}:${dPort}/`); + const dataD = (await responseD.json()) as { count: number; id: string }; + expect(dataD.count).toEqual(4); + const responseA2 = await a.fetch(`/`); + const dataA2 = (await responseA2.json()) as { count: number; id: string }; + expect(dataA2.count).toEqual(5); + expect(dataA.id).toEqual(dataB.id); + expect(dataA.id).toEqual(dataC.id); + expect(dataA.id).toEqual(dataA2.id); }); - const dataA = (await responseA.json()) as { count: number; id: string }; - expect(dataA.count).toEqual(1); - const responseB = await b.fetch(`/`); - const dataB = (await responseB.json()) as { count: number; id: string }; - expect(dataB.count).toEqual(2); - const responseC = await c.fetch(`/`); - const dataC = (await responseC.json()) as { count: number; id: string }; - expect(dataC.count).toEqual(3); - const responseD = await fetch(`http://${dIP}:${dPort}/`); - const dataD = (await responseD.json()) as { count: number; id: string }; - expect(dataD.count).toEqual(4); - const responseA2 = await a.fetch(`/`); - const dataA2 = (await responseA2.json()) as { count: number; id: string }; - expect(dataA2.count).toEqual(5); - expect(dataA.id).toEqual(dataB.id); - expect(dataA.id).toEqual(dataC.id); - expect(dataA.id).toEqual(dataA2.id); - }); -}); + }, + { retry: 2 } +); From 880bc4531df5c823592b7c3ba15913eb5e2d4e3e Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 21 Nov 2023 16:03:31 +0000 Subject: [PATCH 20/24] C3 e2e - drop docusaurus and react tests These jobs tend to flake out and are not providing much of a useful signal. --- packages/create-cloudflare/e2e-tests/frameworks.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/create-cloudflare/e2e-tests/frameworks.test.ts b/packages/create-cloudflare/e2e-tests/frameworks.test.ts index 62703dfaa67..bb588cb7882 100644 --- a/packages/create-cloudflare/e2e-tests/frameworks.test.ts +++ b/packages/create-cloudflare/e2e-tests/frameworks.test.ts @@ -40,6 +40,7 @@ describe.concurrent(`E2E: Web frameworks`, () => { expectResponseToContain: "Dinosaurs are cool", unsupportedPms: ["bun"], testCommitMessage: true, + unsupportedOSs: ["win32"], timeout: LONG_TIMEOUT, }, angular: { @@ -98,6 +99,7 @@ describe.concurrent(`E2E: Web frameworks`, () => { react: { expectResponseToContain: "React App", testCommitMessage: true, + unsupportedOSs: ["win32"], timeout: LONG_TIMEOUT, }, solid: { From 9d82d89d983560591b1c919a33e2859782b03f4a Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 21 Nov 2023 17:23:01 +0000 Subject: [PATCH 21/24] ci: skip fixture tests that exercise the "dev registry" Reenable when https://github.com/cloudflare/workers-sdk/pull/4241 lands and improves reliability of this test. --- fixtures/external-durable-objects-app/tests/index.test.ts | 2 ++ fixtures/service-bindings-app/tests/index.test.ts | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/fixtures/external-durable-objects-app/tests/index.test.ts b/fixtures/external-durable-objects-app/tests/index.test.ts index b43b7f4be95..f384663e56c 100644 --- a/fixtures/external-durable-objects-app/tests/index.test.ts +++ b/fixtures/external-durable-objects-app/tests/index.test.ts @@ -5,6 +5,8 @@ import { describe, expect, it, beforeAll, afterAll } from "vitest"; import type { ChildProcess } from "child_process"; import { type UnstableDevWorker, unstable_dev } from "wrangler"; +// TODO: reenable when https://github.com/cloudflare/workers-sdk/pull/4241 lands +// and improves reliability of this test. describe.skip( "Pages Functions", () => { diff --git a/fixtures/service-bindings-app/tests/index.test.ts b/fixtures/service-bindings-app/tests/index.test.ts index ca66bdd28d1..d7e2bb1ec3b 100644 --- a/fixtures/service-bindings-app/tests/index.test.ts +++ b/fixtures/service-bindings-app/tests/index.test.ts @@ -1,7 +1,10 @@ import { describe, expect, it, beforeAll, afterAll } from "vitest"; import { UnstableDevWorker, unstable_dev } from "wrangler"; import path from "node:path"; -describe("Service Bindings", () => { + +// TODO: reenable when https://github.com/cloudflare/workers-sdk/pull/4241 lands +// and improves reliability of this test. +describe.skip("Service Bindings", () => { let aWorker: UnstableDevWorker; let bWorker: UnstableDevWorker; From 799d3fa300a72be6672d27a1fe50228d8d946572 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 23 Nov 2023 17:47:36 +0000 Subject: [PATCH 22/24] C3 - ensure shell commands in logging are reasonably quoted C3 often outputs log messages to the user of commands that are being executed. Users tend to cut and paste these into their terminal to run themselves. This makes sure that these are likely to just work went pasted into their shell. --- .vscode/settings.json | 2 +- .../create-cloudflare/e2e-tests/helpers.ts | 6 ++- packages/create-cloudflare/package.json | 2 + packages/create-cloudflare/src/common.ts | 47 +++++++++++++++---- .../src/frameworks/angular/index.ts | 4 +- .../src/frameworks/hono/index.ts | 4 +- .../src/frameworks/qwik/index.ts | 3 +- .../src/frameworks/remix/index.ts | 2 +- .../create-cloudflare/src/helpers/command.ts | 3 +- packages/create-cloudflare/src/pages.ts | 7 +-- packages/create-cloudflare/src/types.ts | 4 +- pnpm-lock.yaml | 6 +++ 12 files changed, 67 insertions(+), 23 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index e643e60bb6b..faf41b66dc7 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -29,6 +29,7 @@ "pgrep", "PKCE", "Positionals", + "qwik", "reinitialise", "scandir", "selfsigned", @@ -46,7 +47,6 @@ "websockets", "workerd", "xxhash", - "workerd", "zjcompt" ], "cSpell.ignoreWords": [ diff --git a/packages/create-cloudflare/e2e-tests/helpers.ts b/packages/create-cloudflare/e2e-tests/helpers.ts index 419b97bb1b9..3889b07b1e9 100644 --- a/packages/create-cloudflare/e2e-tests/helpers.ts +++ b/packages/create-cloudflare/e2e-tests/helpers.ts @@ -16,6 +16,7 @@ import { detectPackageManager } from "helpers/packages"; import { fetch } from "undici"; import { expect } from "vitest"; import { version } from "../package.json"; +import { quoteShellArgs } from "../src/common"; import type { Suite, TestContext } from "vitest"; export const C3_E2E_PREFIX = "c3-e2e-"; @@ -81,7 +82,10 @@ export const runC3 = async ({ ); logStream.write( - `Running C3 with command: \`${cmd} ${args.join(" ")}\` (using ${pm})\n\n` + `Running C3 with command: \`${quoteShellArgs([ + cmd, + ...args, + ])}\` (using ${pm})\n\n` ); await new Promise((resolve, rejects) => { diff --git a/packages/create-cloudflare/package.json b/packages/create-cloudflare/package.json index 5270fccd712..325d2e86f11 100644 --- a/packages/create-cloudflare/package.json +++ b/packages/create-cloudflare/package.json @@ -55,6 +55,7 @@ "@types/esprima": "^4.0.3", "@types/node": "^18.15.3", "@types/semver": "^7.5.1", + "@types/shell-quote": "^1.7.2", "@types/which-pm-runs": "^1.0.0", "@types/yargs": "^17.0.22", "@typescript-eslint/eslint-plugin": "^5.55.0", @@ -71,6 +72,7 @@ "pnpm": "^8.10.0", "recast": "^0.22.0", "semver": "^7.5.1", + "shell-quote": "^1.8.1", "typescript": "^5.0.2", "undici": "5.20.0", "vite-tsconfig-paths": "^4.0.8", diff --git a/packages/create-cloudflare/src/common.ts b/packages/create-cloudflare/src/common.ts index 590169c2904..057afd4cd50 100644 --- a/packages/create-cloudflare/src/common.ts +++ b/packages/create-cloudflare/src/common.ts @@ -11,7 +11,7 @@ import { startSection, updateStatus, } from "@cloudflare/cli"; -import { brandColor, dim, gray, bgGreen, blue } from "@cloudflare/cli/colors"; +import { bgGreen, blue, brandColor, dim, gray } from "@cloudflare/cli/colors"; import { inputPrompt, spinner } from "@cloudflare/cli/interactive"; import { getFrameworkCli } from "frameworks/index"; import { processArgument } from "helpers/args"; @@ -25,6 +25,7 @@ import { } from "helpers/command"; import { detectPackageManager } from "helpers/packages"; import { poll } from "helpers/poll"; +import { quote } from "shell-quote"; import { version as wranglerVersion } from "wrangler/package.json"; import { version } from "../package.json"; import type { C3Args, PagesGeneratorContext } from "types"; @@ -137,9 +138,11 @@ export const setupProjectDirectory = (args: C3Args) => { export const offerToDeploy = async (ctx: PagesGeneratorContext) => { startSection(`Deploy with Cloudflare`, `Step 3 of 3`); - const label = `deploy via \`${npm} run ${ - ctx.framework?.config.deployCommand ?? "deploy" - }\``; + const label = `deploy via \`${quoteShellArgs([ + npm, + "run", + ...(ctx.framework?.config.deployCommand ?? ["deploy"]), + ])}\``; ctx.args.deploy = await processArgument(ctx.args, "deploy", { type: "confirm", @@ -166,7 +169,7 @@ export const runDeploy = async (ctx: PagesGeneratorContext) => { const baseDeployCmd = [ npm, "run", - ctx.framework?.config.deployCommand ?? "deploy", + ...(ctx.framework?.config.deployCommand ?? ["deploy"]), ]; const insideGitRepo = await isInsideGitRepo(ctx.project.path); @@ -189,7 +192,7 @@ export const runDeploy = async (ctx: PagesGeneratorContext) => { env: { CLOUDFLARE_ACCOUNT_ID: ctx.account.id, NODE_ENV: "production" }, startText: "Deploying your application", doneText: `${brandColor("deployed")} ${dim( - `via \`${baseDeployCmd.join(" ")}\`` + `via \`${quoteShellArgs(baseDeployCmd)}\`` )}`, }); @@ -255,11 +258,19 @@ export const printSummary = async (ctx: PagesGeneratorContext) => { ], [ `Run the development server`, - `${npm} run ${ctx.framework?.config.devCommand ?? "start"}`, + quoteShellArgs([ + npm, + "run", + ...(ctx.framework?.config.devCommand ?? ["start"]), + ]), ], [ `Deploy your application`, - `${npm} run ${ctx.framework?.config.deployCommand ?? "deploy"}`, + quoteShellArgs([ + npm, + "run", + ...(ctx.framework?.config.deployCommand ?? ["deploy"]), + ]), ], [ `Read the documentation`, @@ -288,7 +299,11 @@ export const printSummary = async (ctx: PagesGeneratorContext) => { `${bgGreen(" APPLICATION CREATED ")}`, `${dim(`Deploy your application with`)}`, `${blue( - `${npm} run ${ctx.framework?.config.deployCommand ?? "deploy"}` + quoteShellArgs([ + npm, + "run", + ...(ctx.framework?.config.deployCommand ?? ["deploy"]), + ]) )}`, ].join(" "); logRaw(msg); @@ -543,3 +558,17 @@ export async function getProductionBranch(cwd: string) { function prepareCommitMessage(commitMessage: string): string { return JSON.stringify(commitMessage); } + +export function quoteShellArgs(args: string[]): string { + if (process.platform !== "win32") { + return quote(args); + } else { + // Simple quoting if there is a space (doesn't handle quotes and spaces together) + const specialCharsMatcher = /[&<>[\]|{}^=;!'+,`~\s]/; + return args + .map((arg) => + arg.match(specialCharsMatcher) ? `"${arg.replaceAll(`"`, `""`)}"` : arg + ) + .join(" "); + } +} diff --git a/packages/create-cloudflare/src/frameworks/angular/index.ts b/packages/create-cloudflare/src/frameworks/angular/index.ts index 49224fd2073..e771590350e 100644 --- a/packages/create-cloudflare/src/frameworks/angular/index.ts +++ b/packages/create-cloudflare/src/frameworks/angular/index.ts @@ -33,8 +33,8 @@ const config: FrameworkConfig = { "pages:build": `ng build && ${npm} run process`, deploy: `${npm} run pages:build && wrangler pages deploy dist/cloudflare`, }), - deployCommand: "deploy", - devCommand: "start", + deployCommand: ["deploy"], + devCommand: ["start"], testFlags: ["--style", "sass"], }; export default config; diff --git a/packages/create-cloudflare/src/frameworks/hono/index.ts b/packages/create-cloudflare/src/frameworks/hono/index.ts index 05ce6c7f38c..321f16aa737 100644 --- a/packages/create-cloudflare/src/frameworks/hono/index.ts +++ b/packages/create-cloudflare/src/frameworks/hono/index.ts @@ -16,8 +16,8 @@ const config: FrameworkConfig = { generate, displayName: "Hono", getPackageScripts: async () => ({}), - deployCommand: "deploy", - devCommand: "dev", + devCommand: ["dev"], + deployCommand: ["deploy"], type: "workers", }; export default config; diff --git a/packages/create-cloudflare/src/frameworks/qwik/index.ts b/packages/create-cloudflare/src/frameworks/qwik/index.ts index b9a2cc38ea0..6d6d5beee05 100644 --- a/packages/create-cloudflare/src/frameworks/qwik/index.ts +++ b/packages/create-cloudflare/src/frameworks/qwik/index.ts @@ -2,6 +2,7 @@ import { endSection } from "@cloudflare/cli"; import { npmInstall, runCommand, runFrameworkGenerator } from "helpers/command"; import { compatDateFlag } from "helpers/files"; import { detectPackageManager } from "helpers/packages"; +import { quoteShellArgs } from "../../common"; import type { FrameworkConfig, PagesGeneratorContext } from "types"; const { npm, npx } = detectPackageManager(); @@ -17,7 +18,7 @@ const configure = async (ctx: PagesGeneratorContext) => { // Add the pages integration const cmd = [npx, "qwik", "add", "cloudflare-pages"]; - endSection(`Running ${cmd.join(" ")}`); + endSection(`Running ${quoteShellArgs(cmd)}`); await runCommand(cmd); }; diff --git a/packages/create-cloudflare/src/frameworks/remix/index.ts b/packages/create-cloudflare/src/frameworks/remix/index.ts index 6ddaa2f38e7..f4a8285e5fb 100644 --- a/packages/create-cloudflare/src/frameworks/remix/index.ts +++ b/packages/create-cloudflare/src/frameworks/remix/index.ts @@ -21,7 +21,7 @@ const config: FrameworkConfig = { getPackageScripts: async () => ({ "pages:deploy": `${npm} run build && wrangler pages deploy ./public`, }), - devCommand: "dev", + devCommand: ["dev"], testFlags: ["--typescript", "--no-install", "--no-git-init"], }; export default config; diff --git a/packages/create-cloudflare/src/helpers/command.ts b/packages/create-cloudflare/src/helpers/command.ts index b2633493279..656f232dfa0 100644 --- a/packages/create-cloudflare/src/helpers/command.ts +++ b/packages/create-cloudflare/src/helpers/command.ts @@ -5,6 +5,7 @@ import { brandColor, dim } from "@cloudflare/cli/colors"; import { isInteractive, spinner } from "@cloudflare/cli/interactive"; import { spawn } from "cross-spawn"; import { getFrameworkCli } from "frameworks/index"; +import { quoteShellArgs } from "../common"; import { detectPackageManager } from "./packages"; import type { PagesGeneratorContext } from "types"; @@ -195,7 +196,7 @@ export const runFrameworkGenerator = async ( endSection( `Continue with ${ctx.framework?.config.displayName}`, - `via \`${cmd.join(" ").trim()}\`` + `via \`${quoteShellArgs(cmd)}\`` ); if (process.env.VITEST) { diff --git a/packages/create-cloudflare/src/pages.ts b/packages/create-cloudflare/src/pages.ts index 452aea05e08..8f03fe1291c 100644 --- a/packages/create-cloudflare/src/pages.ts +++ b/packages/create-cloudflare/src/pages.ts @@ -23,6 +23,7 @@ import { offerGit, offerToDeploy, printSummary, + quoteShellArgs, runDeploy, setupProjectDirectory, } from "./common"; @@ -37,8 +38,8 @@ const VERIFY_PROJECT_RETRIES = 3; const { npx } = detectPackageManager(); const defaultFrameworkConfig = { - deployCommand: "pages:deploy", - devCommand: "pages:dev", + deployCommand: ["pages:deploy"], + devCommand: ["pages:dev"], }; export const runPagesGenerator = async (args: C3Args) => { @@ -213,7 +214,7 @@ const createProject = async (ctx: PagesGeneratorContext) => { env: { CLOUDFLARE_ACCOUNT_ID }, startText: "Creating Pages project", doneText: `${brandColor("created")} ${dim( - `via \`${cmd.join(" ").trim()}\`` + `via \`${quoteShellArgs(cmd)}\`` )}`, }) ); diff --git a/packages/create-cloudflare/src/types.ts b/packages/create-cloudflare/src/types.ts index 4a71bc68fc7..723659d65f1 100644 --- a/packages/create-cloudflare/src/types.ts +++ b/packages/create-cloudflare/src/types.ts @@ -51,8 +51,8 @@ export type FrameworkConfig = { getPackageScripts: () => Promise< Record >; - deployCommand?: string; - devCommand?: string; + deployCommand?: string[]; + devCommand?: string[]; testFlags?: string[]; compatibilityFlags?: string[]; type?: "pages" | "workers"; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index aa4b17d5326..3779289b699 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -611,6 +611,9 @@ importers: '@types/semver': specifier: ^7.5.1 version: 7.5.1 + '@types/shell-quote': + specifier: ^1.7.2 + version: 1.7.2 '@types/which-pm-runs': specifier: ^1.0.0 version: 1.0.0 @@ -659,6 +662,9 @@ importers: semver: specifier: ^7.5.1 version: 7.5.1 + shell-quote: + specifier: ^1.8.1 + version: 1.8.1 typescript: specifier: ^5.0.2 version: 5.0.3 From aed7c14de0305d9a9a81ab2c51805d60bdde2c88 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Fri, 24 Nov 2023 16:22:47 +0000 Subject: [PATCH 23/24] fixup! C3 - ensure shell commands in logging are reasonably quoted --- packages/create-cloudflare/src/common.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/create-cloudflare/src/common.ts b/packages/create-cloudflare/src/common.ts index 057afd4cd50..89ef8fec977 100644 --- a/packages/create-cloudflare/src/common.ts +++ b/packages/create-cloudflare/src/common.ts @@ -563,7 +563,7 @@ export function quoteShellArgs(args: string[]): string { if (process.platform !== "win32") { return quote(args); } else { - // Simple quoting if there is a space (doesn't handle quotes and spaces together) + // Simple Windows command prompt quoting if there are special characters. const specialCharsMatcher = /[&<>[\]|{}^=;!'+,`~\s]/; return args .map((arg) => From c06c3314a3640de3cabbdf89e6e42c814636d24b Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Fri, 24 Nov 2023 16:24:41 +0000 Subject: [PATCH 24/24] fixup! C3 - ensure shell commands in logging are reasonably quoted --- packages/create-cloudflare/src/helpers/command.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/create-cloudflare/src/helpers/command.ts b/packages/create-cloudflare/src/helpers/command.ts index 656f232dfa0..68d425e12fa 100644 --- a/packages/create-cloudflare/src/helpers/command.ts +++ b/packages/create-cloudflare/src/helpers/command.ts @@ -46,7 +46,7 @@ export const runCommand = async ( ): Promise => { return printAsyncStatus({ useSpinner: opts.useSpinner ?? opts.silent, - startText: opts.startText || command.join(" "), + startText: opts.startText || quoteShellArgs(command), doneText: opts.doneText, promise() { const [executable, ...args] = command;