spinner: do not print escape sequences to non-tty output (#7133)

gpanders · web-flow · commit c46e02dfd7e9 · 2024-11-14T13:20:24.000Z
The workers-sdk defines an `isInteractive` test which (incorrectly) does
not check whether stdout is a TTY. This means that when piping output to
a file or to another process, UI elements like the spinner write ANSI
escape sequences to stdout where they are not properly interpreted.

Wrangler has its own separate interactivity test that does check that
stdout is a TTY. This commit updates workers-sdk to use the
`isInteractive` test from Wrangler (which checks that both stdin and
stdout are TTYs) and then updates Wrangler to use this function. This
both eliminates code duplication and also fixes the problem mentioned
above where escape sequences are written to non-TTY outputs.

In addition, the `logOutput` function that the spinner uses (which uses
code from the 3rd party `log-output` library) _unconditionally_ assumes
that stdout is a TTY (it doesn't even check!) and always emits escape
sequences. So when we are running non-interactively, we must use the
`logRaw` function to avoid emitting escape sequences.

While making this change, I also addressed the TODO on the
`isNonInteractiveOrCI` function by using that function throughout the
wrangler codebase.
diff --git a/.changeset/wild-falcons-talk.md b/.changeset/wild-falcons-talk.md
@@ -0,0 +1,5 @@
+---
+"wrangler": patch
+---
+
+Do not emit escape sequences when stdout is not a TTY
diff --git a/packages/cli/index.ts b/packages/cli/index.ts
@@ -61,7 +61,9 @@ export const logRaw = (msg: string) => {
 
 // A simple stylized log for use within a prompt
 export const log = (msg: string) => {
-	const lines = msg.split("\n").map((ln) => `${gray(shapes.bar)} ${white(ln)}`);
+	const lines = msg
+		.split("\n")
+		.map((ln) => `${gray(shapes.bar)}${ln.length > 0 ? " " + white(ln) : ""}`);
 
 	logRaw(lines.join("\n"));
 };
diff --git a/packages/cli/interactive.ts b/packages/cli/interactive.ts
@@ -23,6 +23,9 @@ import {
 import type { OptionWithDetails } from "./select-list";
 import type { Prompt } from "@clack/core";
 
+// logUpdate writes text to a TTY (it uses escape sequences to move the cursor
+// and clear lines). This function should not be used when running
+// non-interactively.
 const logUpdate = createLogUpdate(stdout);
 
 export type Arg = string | boolean | string[] | undefined | number;
@@ -644,7 +647,10 @@ export const spinner = (
 		start(msg: string, helpText?: string) {
 			helpText ||= ``;
 			currentMsg = msg;
-			startMsg = `${currentMsg} ${dim(helpText)}`;
+			startMsg = currentMsg;
+			if (helpText !== undefined && helpText.length > 0) {
+				startMsg += ` ${dim(helpText)}`;
+			}
 
 			if (isInteractive()) {
 				let index = 0;
@@ -660,7 +666,7 @@ export const spinner = (
 					}
 				}, frameRate);
 			} else {
-				logUpdate(`${leftT} ${startMsg}`);
+				logRaw(`${leftT} ${startMsg}`);
 			}
 		},
 		update(msg: string) {
@@ -678,7 +684,7 @@ export const spinner = (
 				clearLoop();
 			} else {
 				if (msg !== undefined) {
-					logUpdate(`\n${grayBar} ${msg}`);
+					logRaw(`${grayBar} ${msg}`);
 				}
 				newline();
 			}
@@ -710,6 +716,13 @@ export const spinnerWhile = async <T>(opts: {
 	}
 };
 
-export const isInteractive = () => {
-	return process.stdin.isTTY;
-};
+/**
+ * Test whether the process is "interactive".
+ */
+export function isInteractive(): boolean {
+	try {
+		return Boolean(process.stdin.isTTY && process.stdout.isTTY);
+	} catch {
+		return false;
+	}
+}
diff --git a/packages/wrangler/src/__tests__/cli.test.ts b/packages/wrangler/src/__tests__/cli.test.ts
@@ -0,0 +1,21 @@
+import { grayBar, leftT, spinner } from "@cloudflare/cli/interactive";
+import { collectCLIOutput } from "./helpers/collect-cli-output";
+import { useMockIsTTY } from "./helpers/mock-istty";
+
+describe("cli", () => {
+	describe("spinner", () => {
+		const std = collectCLIOutput();
+		const { setIsTTY } = useMockIsTTY();
+		test("does not animate when stdout is not a TTY", async () => {
+			setIsTTY(false);
+			const s = spinner();
+			const startMsg = "Start message";
+			s.start(startMsg);
+			const stopMsg = "Stop message";
+			s.stop(stopMsg);
+			expect(std.out).toEqual(
+				`${leftT} ${startMsg}\n${grayBar} ${stopMsg}\n${grayBar}\n`
+			);
+		});
+	});
+});
diff --git a/packages/wrangler/src/__tests__/cloudchamber/curl.test.ts b/packages/wrangler/src/__tests__/cloudchamber/curl.test.ts
@@ -153,8 +153,8 @@ describe("cloudchamber curl", () => {
 		);
 		expect(std.err).toMatchInlineSnapshot(`""`);
 		expect(std.out).toMatchInlineSnapshot(`
-			"├ Loading account 
-			│ 
+			"├ Loading account
+			│
 			>> Body
 			[
 			    {
@@ -310,7 +310,7 @@ describe("cloudchamber curl", () => {
 			"cloudchamber curl /deployments/v2 --header something:here"
 		);
 		expect(std.err).toMatchInlineSnapshot(`""`);
-		const text = std.out.split("\n").splice(1).join("\n");
+		const text = std.out.split("\n").splice(2).join("\n");
 		const response = JSON.parse(text);
 		expect(response.status).toEqual(500);
 		expect(response.statusText).toEqual("Unhandled Exception");
diff --git a/packages/wrangler/src/cloudchamber/common.ts b/packages/wrangler/src/cloudchamber/common.ts
@@ -7,8 +7,7 @@ import { version as wranglerVersion } from "../../package.json";
 import { readConfig } from "../config";
 import { getConfigCache, purgeConfigCaches } from "../config-cache";
 import { getCloudflareApiBaseUrl } from "../environment-variables/misc-variables";
-import { CI } from "../is-ci";
-import isInteractive from "../is-interactive";
+import { isNonInteractiveOrCI } from "../is-interactive";
 import { logger } from "../logger";
 import {
 	DefaultScopeKeys,
@@ -225,7 +224,7 @@ async function fillOpenAPIConfiguration(config: Config, json: boolean) {
 }
 
 export function interactWithUser(config: { json?: boolean }): boolean {
-	return !config.json && isInteractive() && !CI.isCI();
+	return !config.json && !isNonInteractiveOrCI();
 }
 
 type NonObject = undefined | null | boolean | string | number;
diff --git a/packages/wrangler/src/config-cache.ts b/packages/wrangler/src/config-cache.ts
@@ -1,8 +1,7 @@
 import { mkdirSync, readFileSync, rmSync, writeFileSync } from "fs";
 import * as path from "path";
 import { findUpSync } from "find-up";
-import { CI } from "./is-ci";
-import isInteractive from "./is-interactive";
+import { isNonInteractiveOrCI } from "./is-interactive";
 import { logger } from "./logger";
 
 let cacheMessageShown = false;
@@ -32,7 +31,7 @@ const arrayFormatter = new Intl.ListFormat("en-US", {
 });
 
 function showCacheMessage(fields: string[], folder: string) {
-	if (!cacheMessageShown && isInteractive() && !CI.isCI()) {
+	if (!cacheMessageShown && !isNonInteractiveOrCI()) {
 		if (fields.length > 0) {
 			logger.debug(
 				`Retrieving cached values for ${arrayFormatter.format(
diff --git a/packages/wrangler/src/d1/migrations/apply.ts b/packages/wrangler/src/d1/migrations/apply.ts
@@ -5,8 +5,7 @@ import { printWranglerBanner } from "../..";
 import { withConfig } from "../../config";
 import { confirm } from "../../dialogs";
 import { UserError } from "../../errors";
-import { CI } from "../../is-ci";
-import isInteractive from "../../is-interactive";
+import { isNonInteractiveOrCI } from "../../is-interactive";
 import { logger } from "../../logger";
 import { requireAuth } from "../../user";
 import { createBackup } from "../backups";
@@ -155,7 +154,7 @@ Your database may not be available to serve requests during the migration, conti
 					remote,
 					config,
 					name: database,
-					shouldPrompt: isInteractive() && !CI.isCI(),
+					shouldPrompt: !isNonInteractiveOrCI(),
 					persistTo,
 					command: query,
 					file: undefined,
diff --git a/packages/wrangler/src/d1/migrations/helpers.ts b/packages/wrangler/src/d1/migrations/helpers.ts
@@ -2,8 +2,7 @@ import fs from "node:fs";
 import path from "path";
 import { confirm } from "../../dialogs";
 import { UserError } from "../../errors";
-import { CI } from "../../is-ci";
-import isInteractive from "../../is-interactive";
+import { isNonInteractiveOrCI } from "../../is-interactive";
 import { logger } from "../../logger";
 import { DEFAULT_MIGRATION_PATH } from "../constants";
 import { executeSql } from "../execute";
@@ -110,7 +109,7 @@ const listAppliedMigrations = async ({
 		remote,
 		config,
 		name,
-		shouldPrompt: isInteractive() && !CI.isCI(),
+		shouldPrompt: !isNonInteractiveOrCI(),
 		persistTo,
 		command: `SELECT *
 		FROM ${migrationsTableName}
@@ -178,7 +177,7 @@ export const initMigrationsTable = async ({
 		remote,
 		config,
 		name,
-		shouldPrompt: isInteractive() && !CI.isCI(),
+		shouldPrompt: !isNonInteractiveOrCI(),
 		persistTo,
 		command: `CREATE TABLE IF NOT EXISTS ${migrationsTableName}(
 		id         INTEGER PRIMARY KEY AUTOINCREMENT,
diff --git a/packages/wrangler/src/is-interactive.ts b/packages/wrangler/src/is-interactive.ts
@@ -1,3 +1,4 @@
+import { isInteractive as __isInteractive } from "@cloudflare/cli/interactive";
 import { CI } from "./is-ci";
 
 /**
@@ -10,14 +11,12 @@ export default function isInteractive(): boolean {
 		return false;
 	}
 
-	try {
-		return Boolean(process.stdin.isTTY && process.stdout.isTTY);
-	} catch {
-		return false;
-	}
+	return __isInteractive();
 }
 
-// TODO: Use this function across the codebase.
+/**
+ * Test whether a process is non-interactive or running in CI.
+ */
 export function isNonInteractiveOrCI(): boolean {
 	return !isInteractive() || CI.isCI();
 }
diff --git a/packages/wrangler/src/metrics/metrics-config.ts b/packages/wrangler/src/metrics/metrics-config.ts
@@ -6,8 +6,7 @@ import { getConfigCache, saveToConfigCache } from "../config-cache";
 import { confirm } from "../dialogs";
 import { getWranglerSendMetricsFromEnv } from "../environment-variables/misc-variables";
 import { getGlobalWranglerConfigPath } from "../global-wrangler-config-path";
-import { CI } from "../is-ci";
-import isInteractive from "../is-interactive";
+import { isNonInteractiveOrCI } from "../is-interactive";
 import { logger } from "../logger";
 import { getAPIToken } from "../user";
 
@@ -103,7 +102,7 @@ export async function getMetricsConfig({
 
 	// We couldn't get the metrics permission from the project-level nor the user-level config.
 	// If we are not interactive or in a CI build then just bail out.
-	if (!isInteractive() || CI.isCI()) {
+	if (isNonInteractiveOrCI()) {
 		return { enabled: false, deviceId, userId };
 	}
 
diff --git a/packages/wrangler/src/user/user.ts b/packages/wrangler/src/user/user.ts
@@ -223,8 +223,7 @@ import { NoDefaultValueProvided, select } from "../dialogs";
 import { getCloudflareApiEnvironmentFromEnv } from "../environment-variables/misc-variables";
 import { UserError } from "../errors";
 import { getGlobalWranglerConfigPath } from "../global-wrangler-config-path";
-import { CI } from "../is-ci";
-import isInteractive from "../is-interactive";
+import { isNonInteractiveOrCI } from "../is-interactive";
 import { logger } from "../logger";
 import openInBrowser from "../open-in-browser";
 import { parseTOML, readFileSync } from "../parse";
@@ -919,11 +918,10 @@ export async function loginOrRefreshIfRequired(
 ): Promise<boolean> {
 	// TODO: if there already is a token, then try refreshing
 	// TODO: ask permission before opening browser
-	const { isCI } = CI;
 	if (!getAPIToken()) {
 		// Not logged in.
 		// If we are not interactive, we cannot ask the user to login
-		return isInteractive() && !isCI() && (await login(props));
+		return !isNonInteractiveOrCI() && (await login(props));
 	} else if (isAccessTokenExpired()) {
 		// We're logged in, but the refresh token seems to have expired,
 		// so let's try to refresh it
@@ -933,7 +931,7 @@ export async function loginOrRefreshIfRequired(
 			return true;
 		} else {
 			// If the refresh token isn't valid, then we ask the user to login again
-			return isInteractive() && !isCI() && (await login(props));
+			return !isNonInteractiveOrCI() && (await login(props));
 		}
 	} else {
 		return true;
@@ -1193,7 +1191,7 @@ export async function requireAuth(config: {
 }): Promise<string> {
 	const loggedIn = await loginOrRefreshIfRequired();
 	if (!loggedIn) {
-		if (!isInteractive() || CI.isCI()) {
+		if (isNonInteractiveOrCI()) {
 			throw new UserError(
 				"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."
 			);
diff --git a/packages/wrangler/src/versions/deploy.ts b/packages/wrangler/src/versions/deploy.ts
@@ -11,8 +11,7 @@ import {
 import { fetchResult } from "../cfetch";
 import { findWranglerToml, readConfig } from "../config";
 import { UserError } from "../errors";
-import { CI } from "../is-ci";
-import isInteractive from "../is-interactive";
+import { isNonInteractiveOrCI } from "../is-interactive";
 import { logger } from "../logger";
 import * as metrics from "../metrics";
 import { writeOutput } from "../output";
@@ -272,8 +271,8 @@ export async function confirmLatestDeploymentOverwrite(
 				type: "confirm",
 				question: `"wrangler deploy" will upload a new version and deploy it globally immediately.\nAre you sure you want to continue?`,
 				label: "",
-				defaultValue: !isInteractive() || CI.isCI(), // defaults to true in CI for back-compat
-				acceptDefault: !isInteractive() || CI.isCI(),
+				defaultValue: isNonInteractiveOrCI(), // defaults to true in CI for back-compat
+				acceptDefault: isNonInteractiveOrCI(),
 			});
 		}
 	} catch (e) {

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +"wrangler": patch
 +---
++
 +Do not emit escape sequences when stdout is not a TTY