Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use shell-quote package #4080

Merged
merged 5 commits into from
Oct 3, 2023
Merged

use shell-quote package #4080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eca7f30
use `shell-quote` package
RamIdeas Oct 1, 2023
a99f872
wrap shell-quote.parse to match our needs:
RamIdeas Oct 1, 2023
d79639c
fix tests with incorrect CLI args formatting
RamIdeas Oct 1, 2023
4bafbac
fix linting
RamIdeas Oct 2, 2023
ea0e5e7
fix linting in c3
RamIdeas Oct 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/create-cloudflare/e2e-tests/cli.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { tmpdir } from "os";
import { join } from "path";
import { beforeEach, afterEach, describe, test, expect } from "vitest";
import { version } from "../package.json";
import * as shellquote from "../src/helpers/shell-quote";
import { frameworkToTest } from "./frameworkToTest";
import { isQuarantineMode, keys, runC3 } from "./helpers";

Expand Down Expand Up @@ -30,13 +31,15 @@ describe.skipIf(frameworkToTest || isQuarantineMode())(
});

test("--version with positionals", async () => {
const argv = "foo bar baz --version".split(" ");
const argv = shellquote.parse("foo bar baz --version");
const { output } = await runC3({ argv });
expect(output).toEqual(version);
});

test("--version with flags", async () => {
const argv = "foo --type webFramework --no-deploy --version".split(" ");
const argv = shellquote.parse(
"foo --type webFramework --no-deploy --version"
);
const { output } = await runC3({ argv });
expect(output).toEqual(version);
});
Expand Down
8 changes: 5 additions & 3 deletions packages/create-cloudflare/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
"devDependencies": {
"@babel/parser": "^7.21.3",
"@babel/types": "^7.21.4",
"@clack/core": "^0.3.2",
"@clack/prompts": "^0.6.3",
"@cloudflare/eslint-config-worker": "*",
"@cloudflare/workers-tsconfig": "workspace:*",
Expand All @@ -51,6 +52,8 @@
"@types/dns2": "^2.0.3",
"@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",
Expand All @@ -67,16 +70,15 @@
"pnpm": "^8.6.11",
"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",
"vitest": "^0.30.0",
"which-pm-runs": "^1.1.0",
"wrangler": "workspace:*",
"yargs": "^17.7.1",
"yarn": "^1.22.19",
"@clack/core": "^0.3.2",
"@types/semver": "^7.5.1"
"yarn": "^1.22.19"
},
"engines": {
"node": ">=16.13.0"
Expand Down
3 changes: 2 additions & 1 deletion packages/create-cloudflare/src/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ 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();
Expand Down Expand Up @@ -119,7 +120,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 \`${shellquote.quote(baseDeployCmd)}\``
)}`,
});

Expand Down
5 changes: 3 additions & 2 deletions packages/create-cloudflare/src/helpers/__tests__/command.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ 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;
Expand Down Expand Up @@ -57,7 +58,7 @@ describe("Command Helpers", () => {
});

const expectSpawnWith = (cmd: string) => {
const [command, ...args] = cmd.split(" ");
const [command, ...args] = shellquote.parse(cmd);

expect(spawn).toHaveBeenCalledWith(command, args, {
stdio: "inherit",
Expand All @@ -66,7 +67,7 @@ describe("Command Helpers", () => {
};

const expectSilentSpawnWith = (cmd: string) => {
const [command, ...args] = cmd.split(" ");
const [command, ...args] = shellquote.parse(cmd);

expect(spawn).toHaveBeenCalledWith(command, args, {
stdio: "pipe",
Expand Down
11 changes: 6 additions & 5 deletions packages/create-cloudflare/src/helpers/command.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { endSection, stripAnsi } from "./cli";
import { brandColor, dim } from "./colors";
import { spinner } from "./interactive";
import { detectPackageManager } from "./packages";
import * as shellquote from "./shell-quote";
import type { PagesGeneratorContext } from "types";

/**
Expand Down Expand Up @@ -47,12 +48,12 @@ export const runCommand = async (
opts: RunOptions = {}
): Promise<string> => {
if (typeof command === "string") {
command = command.trim().replace(/\s+/g, " ").split(" ");
command = shellquote.parse(command);
}

return printAsyncStatus({
useSpinner: opts.useSpinner ?? opts.silent,
startText: opts.startText || command.join(" ").trim(),
startText: opts.startText || shellquote.quote(command),
doneText: opts.doneText,
promise() {
const [executable, ...args] = command;
Expand Down Expand Up @@ -181,7 +182,7 @@ export const runFrameworkGenerator = async (
cmd: string
) => {
if (ctx.framework?.args?.length) {
cmd = `${cmd} ${ctx.framework.args.join(" ")}`;
cmd = `${cmd} ${shellquote.quote(ctx.framework.args)}`;
}

endSection(
Expand All @@ -191,7 +192,7 @@ export const runFrameworkGenerator = async (

if (process.env.VITEST) {
const flags = ctx.framework?.config.testFlags ?? [];
cmd = `${cmd} ${flags.join(" ")}`;
cmd = `${cmd} ${shellquote.quote(flags)}`;
}

await runCommand(cmd);
Expand Down Expand Up @@ -228,7 +229,7 @@ export const installPackages = async (
break;
}

await runCommand(`${npm} ${cmd} ${saveFlag} ${packages.join(" ")}`, {
await runCommand(`${npm} ${cmd} ${saveFlag} ${shellquote.quote(packages)}`, {
...config,
silent: true,
});
Expand Down
34 changes: 34 additions & 0 deletions packages/create-cloudflare/src/helpers/shell-quote.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import shellquote from "shell-quote";

export const quote = shellquote.quote;

export function parse(cmd: string, env?: Record<string, string>): string[] {
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;
}
2 changes: 2 additions & 0 deletions packages/wrangler/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@
"@types/prompts": "^2.0.14",
"@types/react": "^17.0.37",
"@types/serve-static": "^1.13.10",
"@types/shell-quote": "^1.7.2",
"@types/signal-exit": "^3.0.1",
"@types/source-map-support": "^0.5.7",
"@types/supports-color": "^8.1.1",
Expand Down Expand Up @@ -185,6 +186,7 @@
"remove-accents-esm": "^0.0.1",
"semiver": "^1.1.0",
"serve-static": "^1.15.0",
"shell-quote": "^1.8.1",
"shellac": "^0.8.0",
"signal-exit": "^3.0.7",
"strip-ansi": "^7.0.1",
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/src/__tests__/d1/execute.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ describe("execute", () => {
});

await expect(
runWrangler("d1 execute --command 'select 1;'")
runWrangler("d1 execute db --command 'select 1;'")
Copy link
Contributor Author

@RamIdeas RamIdeas Oct 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This test forgot to specify the database positional argument. It was passing before because the command argument has a space (and previously we split the whole command by space), resulting in command parsed as 'select and database parsed as 1;'

).rejects.toThrowError(
`In a non-interactive environment, it's necessary to set a CLOUDFLARE_API_TOKEN environment variable for wrangler to work. Please go to https://developers.cloudflare.com/fundamentals/api/get-started/create-token/ for instructions on how to create an api token, and assign its value to CLOUDFLARE_API_TOKEN.`
);
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/src/__tests__/deploy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4323,7 +4323,7 @@ addEventListener('fetch', event => {});`
mockSubDomainRequest();
mockUploadWorkerRequest();
await runWrangler(
"deploy --dry-run --outdir dist --define abc:'https://www.abc.net.au/news/'"
`deploy --dry-run --outdir dist --define "abc:'https://www.abc.net.au/news/'"`
Copy link
Contributor Author

@RamIdeas RamIdeas Oct 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This wasn't a correct example of how a user would specify the define arg with this value.

Before (incorrect, note the missing quotes by the time node.js sees the value):

▶ node -e 'console.log(process.argv.slice(1))' -- --define abc:'123'
[ '--define', 'abc:123' ]

After (correct, note the quotes in the value as expected):

▶ node -e 'console.log(process.argv.slice(1))' -- --define "abc:'123'"
[ '--define', "abc:'123'" ]

);

expect(fs.readFileSync("dist/index.js", "utf-8")).toContain(
Expand Down
6 changes: 4 additions & 2 deletions packages/wrangler/src/__tests__/helpers/run-wrangler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import { main } from "../../index";
import * as shellquote from "../../utils/shell-quote";
import { normalizeSlashes, stripTimings } from "./mock-console";

/**
* A helper to 'run' wrangler commands for tests.
*/
export async function runWrangler(cmd?: string) {
export async function runWrangler(cmd = "") {
try {
await main(cmd?.split(" ") ?? []);
const argv = shellquote.parse(cmd);
await main(argv);
} catch (err) {
if (err instanceof Error) {
err.message = normalizeSlashes(stripTimings(err.message));
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/src/__tests__/kv.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ describe("wrangler", () => {
},
});
await runWrangler(
`kv:key put dKey dVal --namespace-id some-namespace-id --metadata {"mKey":"mValue"}`
`kv:key put dKey dVal --namespace-id some-namespace-id --metadata '{"mKey":"mValue"}'`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't a correct example of how a user would specify the metadata arg with JSON.

Before (incorrect, note the missing quotes which would fail JSON.parse)

▶ node -e 'console.log(process.argv.slice(1))' --  --metadata {"mKey":"mValue"}  
[ '--metadata', '{mKey:mValue}' ]

After (correct, note the valid JSON as expected):

▶ node -e 'console.log(process.argv.slice(1))' --  --metadata '{"mKey":"mValue"}'
[ '--metadata', '{"mKey":"mValue"}' ]

);
expect(requests.count).toEqual(1);
expect(std.out).toMatchInlineSnapshot(
Expand Down
5 changes: 3 additions & 2 deletions packages/wrangler/src/init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { getPackageManager } from "./package-manager";
import { parsePackageJSON, parseTOML, readFileSync } from "./parse";
import { getBasePath } from "./paths";
import { requireAuth } from "./user";
import * as shellquote from "./utils/shell-quote";
import { CommandLineArgsError, printWranglerBanner } from "./index";

import type { RawConfig } from "./config";
Expand Down Expand Up @@ -184,7 +185,7 @@ export async function initHandler(args: InitArgs) {
}

const c3Arguments = [
...getC3CommandFromEnv().split(" "),
...shellquote.parse(getC3CommandFromEnv()),
Comment on lines -187 to +188
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would have failed in scenarios where the c3 command was pointing to a local executable, for example, in a directory with spaces

fromDashScriptName,
...(yesFlag && isNpm(packageManager) ? ["-y"] : []), // --yes arg for npx
...(isNpm(packageManager) ? ["--"] : []),
Expand Down Expand Up @@ -252,7 +253,7 @@ export async function initHandler(args: InitArgs) {
c3Arguments.unshift("-y"); // arg for npx
}

c3Arguments.unshift(...getC3CommandFromEnv().split(" "));
c3Arguments.unshift(...shellquote.parse(getC3CommandFromEnv()));
Comment on lines -255 to +256
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would have failed in scenarios where the c3 command was pointing to a local executable, for example, in a directory with spaces


// Deprecate the `init --from-dash` command
const replacementC3Command = `\`${packageManager.type} ${c3Arguments.join(
Expand Down
34 changes: 34 additions & 0 deletions packages/wrangler/src/utils/shell-quote.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import shellquote from "shell-quote";

export const quote = shellquote.quote;

export function parse(cmd: string, env?: Record<string, string>): string[] {
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;
}
Comment on lines +1 to +34
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is duplicated across C3 and wrangler. I will extract to a new shared package when #4038 is merged

Loading